feat: mailbox

[pyranota]

No description provided.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	c82e972
Status:	✅  Deploy successful!
Preview URL:	https://846449d5.windmill.pages.dev
Branch Preview URL:	https://feat-mailbox.windmill.pages.dev

View logs

[pyranota]

/updatesqlx

[claude]

Claude finished @pyranota's task —— View job

---

Code Review Complete

I've reviewed the mailbox feature implementation. Here's my comprehensive feedback:

✅ Strengths

1. Good test coverage: The implementation includes comprehensive tests covering multiple scenarios (lines 186-302 in mailbox.rs)
2. Clean API design: The Mailbox struct provides intuitive methods (push, pull, read, read_all, pull_all, delete, delete_batch)
3. Proper database migrations: Both up and down migrations are provided
4. Thread-safe operations: The test demonstrates concurrent mailbox operations work correctly

⚠️ Issues and Concerns

1. Critical: Query Performance Issues

Location: mailbox.rs:62-68 (pull method)

The pull method uses a subquery pattern that can cause performance issues:

DELETE FROM mailbox 
WHERE message_id = (
    SELECT message_id
    FROM mailbox
    WHERE type = $1 AND mailbox_id = $2 AND workspace_id = $3
    LIMIT 1
)

Problem: This uses a subquery without FOR UPDATE SKIP LOCKED, which can cause:

• Lock contention under high concurrency
• Race conditions where multiple workers try to pull the same message
• Deadlocks in high-throughput scenarios

Recommendation: Use FOR UPDATE SKIP LOCKED pattern:

DELETE FROM mailbox 
WHERE message_id = (
    SELECT message_id
    FROM mailbox
    WHERE type = $1 AND mailbox_id = $2 AND workspace_id = $3
    ORDER BY message_id ASC
    LIMIT 1
    FOR UPDATE SKIP LOCKED
)
RETURNING payload, created_at, message_id as id;

2. Missing ORDER BY in pull method

Location: mailbox.rs:62-68

The pull query doesn't specify an ORDER BY clause, meaning messages may not be retrieved in insertion order (FIFO).

Recommendation: Add ORDER BY message_id ASC to ensure FIFO ordering.

3. Inconsistent Formatting

Location: mailbox.rs:63

Line 63 has unusual formatting with strange characters:

WHERE message_id = (                                                                                                                       SELECT message_id                                                                                                  ║

This appears to have non-standard whitespace or special characters. Should be cleaned up.

4. Potential Performance: Missing Index Coverage

Location: 20251028105101_mailbox.up.sql:18-19

The index idx_mailbox_type_mailbox_id_message_id covers (type, mailbox_id, message_id) but several queries also filter by workspace_id:

• pull: filters by type, mailbox_id, workspace_id
• pull_all: filters by type, mailbox_id, workspace_id
• read: filters by type, mailbox_id, workspace_id
• read_all: filters by type, mailbox_id, workspace_id

Recommendation: Consider adding workspace_id to the index:

CREATE INDEX idx_mailbox_lookup 
ON mailbox(workspace_id, type, mailbox_id, message_id ASC);

5. SQLx Best Practice: Missing Explicit Column Selection

Location: mailbox.rs:117, 140 (delete and delete_batch methods)

According to the Rust best practices document, queries executed by workers should avoid SELECT * patterns. The delete and delete_batch methods use .fetch_all() but don't need the result:

.fetch_all(e)  // This tries to fetch rows that were deleted
.await?;

Recommendation: Use .execute() instead since you don't need the returned rows:

.execute(e)
.await?;

6. Database Schema: No Cleanup Strategy

Location: Migration file

The mailbox table has no retention policy or cleanup mechanism. Over time, this could accumulate stale messages if:

• Messages are pushed but never pulled
• A mailbox_id is abandoned
• Errors prevent message consumption

Recommendation: Consider adding:

• A expires_at timestamp column
• A background cleanup job
• Or document the expected lifecycle management strategy

7. Type Naming: Limited Future Extensibility

Location: 20251028105101_mailbox.up.sql:4-7

Only two mailbox types are defined:

CREATE TYPE mailbox_type AS ENUM (
    'trigger',
    'debouncing_stale_data'
);

Question: Are these the only planned use cases? The generic "mailbox" name suggests broader applicability, but the enum is quite specific.

8. Test Issue: Left-in Debug Statement

Location: mailbox.rs:287-292

dbg!(
    sqlx::query!("SELECT mailbox_id, payload, workspace_id FROM mailbox")
        .fetch_all(db)
        .await
        .unwrap()
);

Recommendation: Remove the dbg! statement before merging - it will clutter test output.

9. Migration Ordering Issue

Location: 20251028105101_mailbox.down.sql:5

The down migration tries to drop the index after dropping the table:

DROP TABLE IF EXISTS mailbox;
DROP TYPE IF EXISTS mailbox_type;
DROP SEQUENCE IF EXISTS mailbox_id_seq;
DROP INDEX IF EXISTS idx_mailbox_type_mailbox_id_message_id;  -- This will fail

Problem: Once the table is dropped, the index is automatically dropped too. The explicit index drop will fail.

Recommendation: Remove line 5, or move it before the table drop.

📋 Minor Observations

1. Code style: The code follows Rust conventions well with proper use of sqlx macros
2. Error handling: Proper use of error::Result throughout
3. Type safety: Good use of sqlx::Type for the enum
4. Nullable handling: Correct use of IS NOT DISTINCT FROM for nullable mailbox_id comparisons

🔒 Security Considerations

• ✅ Workspace isolation is properly enforced in all queries
• ✅ No SQL injection risks (parameterized queries throughout)
• ⚠️ No rate limiting or quota checks - a malicious actor could spam messages into a mailbox

🎯 Testing Instructions

To test this feature, navigate to any area of the Windmill app where triggers or debouncing is used. The mailbox system provides a message queue mechanism for these features. Specifically, test scenarios where multiple trigger events arrive rapidly and need to be processed in order, or where debouncing logic needs to track stale data. You can verify the implementation by checking that triggers fire in the correct FIFO order, that concurrent operations don't lose messages, and that messages can be read without consuming them or pulled to consume them atomically. The system should properly isolate messages by workspace and mailbox type.

---

[windmill-internal-app]

Starting sqlx update...

[ellipsis-dev]

Remove the stray '║' character in the SQL query and consider using 'IS NOT DISTINCT FROM' for mailbox_id (to correctly match NULL values). Also, add an ORDER BY (e.g., ORDER BY message_id ASC) in the subquery for deterministic behavior.

Suggested change

	WHERE message_id = ( SELECT message_id ║
	WHERE message_id = ( SELECT message_id

[ellipsis-dev]

Use .execute() instead of .fetch_all() in the delete() method since the returned rows are not used.

Suggested change

	.fetch_all(e)
	.execute(e)

[ellipsis-dev]

Use .execute() instead of .fetch_all() in the delete_batch() method to improve performance by avoiding retrieval of result rows.

Suggested change

	.fetch_all(e)
	.execute(e)

[windmill-internal-app]

Successfully ran sqlx update

[ellipsis-dev]

Remove stray non-ASCII character (‘║’) from the query.

Suggested change

	"query": "\n DELETE FROM mailbox \n WHERE message_id = ( SELECT message_id ║\n FROM mailbox \n WHERE type = $1 AND mailbox_id = $2 AND workspace_id = $3 \n LIMIT 1 \n ) \n RETURNING payload, created_at, message_id as id;\n ",
	"query": "\n DELETE FROM mailbox \n WHERE message_id = ( SELECT message_id \n FROM mailbox \n WHERE type = $1 AND mailbox_id = $2 AND workspace_id = $3 \n LIMIT 1 \n ) \n RETURNING payload, created_at, message_id as id;\n ",