Implement MSC4140 delayed events - #349
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f75e63ce4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// The duration that the server should wait before sending this event. | ||
| #[serde(with = "crate::serde::duration::ms")] | ||
| #[salvo(schema(value_type = u64))] | ||
| pub delay: Duration, |
There was a problem hiding this comment.
MSC4140 defines the scheduling duration as delay_ms, but this field has no rename, so serde requires delay and rejects compliant requests before scheduling. The lookup DTO also serializes delay / running_since instead of delay_ms / scheduled_at, so clients using the advertised org.matrix.msc4140 API cannot parse the implementation's request/response shapes; please rename or alias these timing fields before advertising the feature.
Useful? React with 👍 / 👎.
| "org.matrix.msc4140.delay_id".to_owned(), | ||
| to_raw_value(&event.delay_id)?, |
There was a problem hiding this comment.
Storing org.matrix.msc4140.delay_id in the PDU unsigned map makes it visible to everyone: the client-history paths only call remove_transaction_id() for non-senders, and convert_to_outgoing_federation_event also removes only transaction_id. For delayed events in shared or federated rooms this leaks the sender's opaque delay id to other users, contrary to MSC4140's sender-only unsigned requirement; keep it internal or scrub it unless the requesting user is the event sender.
Useful? React with 👍 / 👎.
Add the current MSC4140 delayed-events API and a persistent scheduler:
- palpo-core: delayed_events client models (DelayedEventData, update
actions, request/response types) matching upstream Ruma's revised API
shapes, feature-gated on unstable-msc4140
- palpo-data: delayed_events table with migration and queries; rows are
claimed atomically before sending so concurrent workers and management
requests cannot double-send
- palpo: client endpoints for scheduling (PUT rooms/{room_id}/
delayed_event/{event_type}/{txn_id}), listing, fetching one, and
restart/send/cancel actions (path-action form plus the deprecated
body-action form); a background scheduler that sends due events
through the normal event authorization and federation paths and
recovers overdue events chronologically after a restart; per-user
scheduled-event and max-delay limits with M_LIMIT_EXCEEDED/Retry-After
and M_FORBIDDEN; idempotent scheduling via transaction ids and
idempotent/conflicting (HTTP 409) action semantics per the MSC
- advertise org.matrix.msc4140 in /versions and the
org.matrix.msc4140.delayed_events limits capability only when the
feature is enabled; new [delayed_events] config section
Power levels are evaluated at the point of sending, as the MSC requires.
Failures at the scheduled send time are recorded on the delayed event
instead of retried; manual send failures keep the event scheduled.
Closes #319
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The scheduler wrote `org.matrix.msc4140.delay_id` into the sent event's `unsigned` and nothing ever took it back out, so every member of the room saw the sender's delay id over /sync, /messages and /context, and it was forwarded verbatim over federation. MSC4140 requires the opposite: the delay_id is included "if, and only if, the client being given the event is authenticated as the event's sender". That is the same rule `transaction_id` already follows here, so route it through the same two chokepoints — rename `Pdu::remove_transaction_id` to `remove_sender_only_unsigned` and strip both fields, and drop it in `convert_to_outgoing_federation_event`. Also make `supported_versions_body` take the delayed-events flag as an argument. It is called directly by unit tests, which have no initialized global config, so reading `config::get()` inside it panicked. Rebased onto main, which had refactored `/versions` into that function. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7f75e63 to
e40686b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e40686bb3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter(delayed_events::id.eq(row_id)) | ||
| .filter(delayed_events::finalized_at.is_null()), | ||
| ) | ||
| .set(delayed_events::finalized_at.eq(now)) |
There was a problem hiding this comment.
When a restart action races with process_due_events() after the latter has called list_due, restart updates send_at successfully, but this claim only checks id and finalized_at. The stale scheduler entry is therefore still claimed and sent immediately despite the successful restart; include the selected due time or a fresh send_at <= now predicate in the atomic claim.
Useful? React with 👍 / 👎.
| .set(delayed_events::finalized_at.eq(now)) | ||
| .get_result::<DbDelayedEvent>(&mut connect().await?) |
There was a problem hiding this comment.
If the process exits after this update but before set_sent or set_error, the row remains permanently marked finalized with neither an event ID nor an error. On restart, both list_due and next_send_at exclude it, so a persisted scheduled event is silently interpreted as cancelled and is never sent; claims need a distinct recoverable processing state or a lease that startup can reclaim.
Useful? React with 👍 / 👎.
| Err(error) => { | ||
| // The MSC requires the event to stay scheduled so the | ||
| // client can retry until the scheduled send time. | ||
| delayed_event::unclaim(claimed.id).await?; |
There was a problem hiding this comment.
For a manual send, an error from send_delayed_pdu does not prove that no PDU was created: build_and_append_pdu persists via append_pdu before later fallible push and federation-queue operations. If one of those downstream operations fails, this unclaim schedules another attempt, which can append a duplicate message event even though the first one is already in the timeline; preserve or recover the created event ID before making the row retryable.
Useful? React with 👍 / 👎.
| send_at: now + delay_ms as i64, | ||
| created_at: now, | ||
| }; | ||
| let row = delayed_event::create(new).await?; |
There was a problem hiding this comment.
When two retries with the same user, device, and transaction ID arrive concurrently, both can pass the preceding lookup and then reach this insert. The unique transaction index makes one insert fail with a database error, producing a 500 instead of returning the winning request's delay_id, so the endpoint is not idempotent under concurrent retries; use an upsert or catch the uniqueness conflict and fetch the existing row.
Useful? React with 👍 / 👎.
| if refreshed.event_id.is_some() { | ||
| Err(finalized_conflict(&refreshed, "cancel")) | ||
| } else { | ||
| // Already cancelled, either by user action or an error. | ||
| Ok(()) |
There was a problem hiding this comment.
When the scheduled send has already finalized with error set and no event_id, this branch returns success and reports the action as an idempotent cancellation. That outcome was a failed send rather than a prior cancellation, so it should take the existing finalized_conflict path; the same branch can also falsely acknowledge cancellation while a claimed send is still in progress and has not populated either outcome field.
Useful? React with 👍 / 👎.
The claim marked a row `finalized_at` up front and used that single column for both "a worker is sending this" and "this reached a final outcome". Five problems followed, all raised in review: - A `restart` landing after `list_due` but before the claim updated `send_at`, but the claim only matched on id, so the stale scheduler entry still sent the event immediately and the restart was silently undone. The scheduler now claims through `claim_due`, which re-checks `send_at <= now`. The manual `send` action keeps a claim without that predicate, since sending ahead of time is its whole purpose. - A process exiting between the claim and `set_sent`/`set_error` left the row finalized with neither outcome. `list_due` and `next_send_at` both skip it, so a scheduled event was silently swallowed and read back as cancelled. Claims are now a `claimed_at` lease that leaves `finalized_at` null, and both queries reclaim leases older than CLAIM_LEASE_MS. - On a manual send, any error unclaimed the row and left it schedulable. But `build_and_append_pdu` persists the event before its later push and federation steps, so a failure there could reschedule an event that was already in the timeline and duplicate it. `send_delayed_pdu` now reports whether it failed before or after the append; only the former is retryable, the latter finalizes with the error. - Two concurrent retries with the same transaction id could both pass the `get_by_txn_id` lookup, and the loser's insert hit the unique index and surfaced as a 500. `create` now resolves a unique violation to the winning row. - `cancel` and `restart` matched any non-finalized row, so they reported success for an event a worker was already sending. Both now exclude live leases and return 409. Cancelling an event that failed to send stays an idempotent success -- the MSC counts an error as having been cancelled "either due to user action or an error". The migration is edited in place rather than followed by an ALTER: it has not been released, it only exists on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed the concurrency and durability findings in 340bab1: claims are now a Two I am not applying, with reasons: "Use MSC timing field names" — the MSC text does say "Return a conflict when cancellation follows a send error" — the MSC explicitly makes this an idempotent success: the homeserver responds with success "if the @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 340bab12c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match send_delayed_pdu(&claimed).await { | ||
| Ok(event_id) => { | ||
| delayed_event::set_sent(claimed.id, &event_id, now).await?; |
There was a problem hiding this comment.
If the process exits after send_delayed_pdu has persisted the PDU but before set_sent records the outcome, the delayed row retains only its lease; after five minutes list_due reclaims it and calls build_and_append_pdu again. Even if add_txn_id completed before the crash, the retry path never consults that mapping, so ordinary message events can be appended twice. The append and durable send outcome need an idempotent recovery mechanism rather than relying solely on lease expiry.
Useful? React with 👍 / 👎.
| .await | ||
| .map_err(SendFailure::Appending)? |
There was a problem hiding this comment.
When a manual send encounters an authorization failure inside PduBuilder::hash_sign, no PDU has been written yet, but mapping every build_and_append_pdu error to Appending sends it through the branch that permanently finalizes the delayed event. This occurs, for example, when room membership or power levels currently reject the event; the client then cannot retry after authorization changes even though retrying was safe. The failure boundary needs to distinguish errors before hash_sign_save persists the PDU from errors during or after persistence.
Useful? React with 👍 / 👎.
| delayed_events::table | ||
| .filter(delayed_events::finalized_at.is_null()) | ||
| .select(diesel::dsl::min(delayed_events::send_at)) |
There was a problem hiding this comment.
When a due row has an unexpired claimed_at lease, list_due correctly excludes it, but this query still returns its already-past send_at. The scheduler consequently computes a zero-duration sleep and repeatedly runs list_due and next_send_at; this can hammer the database for the full five-minute lease after a restart, or whenever another server instance owns the send. Compute the next eligible time using the lease expiry or omit actively leased rows.
Useful? React with 👍 / 👎.
| let scheduled = delayed_event::count_scheduled(user_id).await?; | ||
| if scheduled >= conf.delayed_events.max_scheduled as i64 { |
There was a problem hiding this comment.
The per-user limit is checked separately from insertion, so concurrent requests with distinct transaction IDs can all observe a count below max_scheduled and then all insert successfully. For example, ten requests arriving when the count is 99 can raise a configured limit of 100 to 109; the unique transaction index does not serialize distinct transactions. The count check and insertion need per-user serialization or a database-level atomic constraint.
Useful? React with 👍 / 👎.
The previous round fixed crash recovery by making claims a lease, which traded one bug for another: a process dying between the append and `set_sent` left a row the sweep reclaimed and sent a second time. Lease expiry alone is not idempotent. Resolve the transaction id before appending, the same way `/send` does. The mapping is written right after the append, so a hit means the event already reached the room and recovery just records it. The residual window -- a crash between the append and that mapping -- is the one palpo's ordinary `/send` idempotency already has, rather than a new one. That also removes the reason for splitting send failures by whether they happened before or after the append. Classifying every `build_and_append_pdu` error as post-append was wrong anyway: an authorization rejection writes nothing, and permanently finalizing the delayed event on one denied the client the retry the MSC promises. With recovery idempotent, releasing the lease on any error is safe again. Two more from the same review: - `next_send_at` returned the past `send_at` of leased rows that `list_due` skips, so the scheduler computed a zero sleep and spun on the database for the whole lease. It now skips live leases, and a companion query reports the next lease expiry so the sweep still wakes up to reclaim. - The per-user `max_scheduled` check ran outside the insert, so concurrent requests with distinct transaction ids could all pass it and push the count past the limit. Count and insert now share a transaction guarded by a per-user advisory lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in 9bb4ed6. The lease fix from the last round did trade one bug for another, as flagged — expiry alone is not idempotent. That also resolves the authorization finding: with recovery idempotent there is no longer a reason to classify failures by whether they happened before or after the append, so the
Two repeats from the previous round, unchanged:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bb4ed6147
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| query | ||
| .first::<DbDelayedEvent>(&mut *conn) | ||
| .await |
There was a problem hiding this comment.
When two retries with the same transaction ID both pass the caller's pre-check and the first commits while the second waits for the advisory lock, the second insert raises a PostgreSQL unique violation. PostgreSQL then leaves this transaction aborted, so the following first query fails with 25P02 rather than returning AlreadyScheduled, causing the retry to receive a server error. Use ON CONFLICT DO NOTHING, a savepoint/nested transaction, or recheck the transaction ID after taking the advisory lock instead of querying after the failed statement.
Useful? React with 👍 / 👎.
| if scheduled >= max_scheduled { | ||
| return Ok(Scheduled::LimitReached); |
There was a problem hiding this comment.
If the first of two concurrent identical scheduling requests consumes the user's final available slot, the second request can miss it in the pre-transaction lookup, wait for the advisory lock, and then return LimitReached here without resolving the now-committed row. The same transaction therefore receives M_LIMIT_EXCEEDED instead of its original delay_id; recheck (user, device, txn_id) after acquiring the lock and before counting.
Useful? React with 👍 / 👎.
| /// record it. The remaining window -- a crash between the append and that | ||
| /// mapping -- is the one palpo's ordinary `/send` idempotency already has. | ||
| async fn send_delayed_pdu(event: &DbDelayedEvent) -> AppResult<OwnedEventId> { | ||
| if let Some(event_id) = crate::transaction_id::get_event_id( |
There was a problem hiding this comment.
If the original worker remains alive beyond the five-minute lease—for example while waiting on the room mutex—a second worker can reclaim the row and perform this lookup before the first worker records its transaction mapping. Both then proceed past the check, and after the first append completes the reclaimed worker can append the event again because it never rechecks or holds a durable reservation. Lease reclamation therefore still permits duplicate room events; use a fenced claim or atomic transaction reservation that prevents two reclaimed workers from reaching the append concurrently.
Useful? React with 👍 / 👎.
Three more from review, one of them introduced by the previous commit: - Catching the unique violation inside the new transaction does not work: Postgres aborts the transaction on the conflict, so the refetch that followed it would fail with 25P02 and the retry would get a 500 instead of its delay id. Re-resolve the transaction id after taking the advisory lock instead, before counting and inserting, so the insert can no longer conflict at all. - That same reordering fixes the other half: a retry whose twin had just consumed the user's last slot used to fall through to the limit check and be told M_LIMIT_EXCEEDED rather than being handed the delay id it had already been assigned. - The transaction-id lookup was a bare check-then-act. A worker that reclaimed an over-running lease -- the original being alive but stuck waiting on the room mutex -- could pass it while that original was between its append and its `add_txn_id`, and both would append. The lookup now happens under the room lock and `add_txn_id` is recorded before the lock is released, so the check and the write it depends on are ordered by the same mutex that orders the appends. Outcome writes are also fenced on the lease value now, so a worker whose lease was reclaimed while it was still running cannot overwrite the outcome its successor recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in the latest push. All three were right, and the first was mine from the previous round — catching the unique violation inside the transaction is useless in Postgres because the conflict aborts the transaction, so the refetch would have hit 25P02. Re-resolving the transaction id right after the advisory lock removes the conflict entirely, and it also fixes the limit interaction you flagged. For the fencing: the txn-id lookup now runs under the room lock and The two long-standing repeats ( @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85eead66d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async fn process_due_events() -> AppResult<()> { | ||
| let now = UnixMillis::now().get() as i64; | ||
| for event in delayed_event::list_due(now).await? { | ||
| let Some(claimed) = delayed_event::claim_due(event.id, now).await? else { |
There was a problem hiding this comment.
When processing a backlog takes longer than CLAIM_LEASE_MS, this passes the scan-start timestamp to later claims, so a newly claimed row is immediately considered expired. A concurrent management request can then report a successful cancel/restart while the event is being appended, and another server instance can reclaim it concurrently; obtain a fresh timestamp for each claim and for its final outcome.
Useful? React with 👍 / 👎.
| ); | ||
| unsigned.insert("transaction_id".to_owned(), to_raw_value(&event.txn_id)?); | ||
|
|
||
| let state_lock = room::lock_state(&event.room_id).await; |
There was a problem hiding this comment.
In a multi-instance deployment sharing this database, room::lock_state is only a process-local static mutex (crates/server/src/room.rs), so it does not order this lookup and append against a worker on another instance. If a live worker's lease expires, both instances can pass get_event_id before either writes the mapping and append duplicate events; reclaimed sends need a cross-process lock or another database-backed append fence.
Useful? React with 👍 / 👎.
| diesel::update( | ||
| delayed_events::table | ||
| .filter(delayed_events::id.eq(row_id)) | ||
| .filter(delayed_events::claimed_at.eq(lease)), |
There was a problem hiding this comment.
If a worker remains alive after its lease expires, restart and cancel can update the row but leave claimed_at unchanged; this predicate therefore still matches when that old worker completes, allowing set_sent (and the identical set_error predicate) to overwrite the management action's outcome. The winning transition must invalidate the prior lease, and outcome writes should also require the row to remain unfinalized.
Useful? React with 👍 / 👎.
| .await | ||
| .ok(); |
There was a problem hiding this comment.
If this insert fails after the PDU append, the function still reports success without durably establishing the idempotency marker. Should recording the delayed-event outcome then fail or the process stop before it, lease recovery finds neither an outcome nor a transaction mapping and appends the event again; keep the room lock until the mapping is durably recorded and handle or retry this error rather than dropping it.
Useful? React with 👍 / 👎.
Three more from review: - `process_due_events` passed its scan-start timestamp to every claim, so working through a backlog longer than CLAIM_LEASE_MS stamped already-expired leases and a management request could report a successful cancel while the event was being appended. The clock is now re-read per row. - `restart` and `cancel` left `claimed_at` untouched, so a worker that outlived its lease still matched the fence and could record an outcome over a row that had since been rescheduled or cancelled. Both now clear the lease. - `add_txn_id` failures were swallowed with `.ok()`. That mapping is what makes lease recovery idempotent, so losing it silently and then failing to record the outcome would let recovery append the event twice. It now propagates. Not fixed, and not fixable at this layer: `room::lock_state` is a process-local mutex, so it orders the transaction-id check against the append only within one instance. Two instances sharing this database can still both reclaim an expired lease and append. Closing that needs the claim and the append to share a database-level fence -- a `FOR UPDATE SKIP LOCKED` worker queue or an equivalent -- rather than the current advisory-lock-plus-in-process-mutex pairing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed the three tractable ones in 5535adc: the claim timestamp is re-read per row, "Fence reclaimed sends across server processes" — confirmed, and not fixable by patching. You are right that Every round so far has surfaced a deeper defect in the same mechanism, and two of them were introduced by the previous round's fix. That is the signal that this is a design mismatch rather than a bug list: a cross-process scheduler is being built on primitives ( Flagging for a maintainer decision rather than continuing to iterate. Note |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5535adc9ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn remove_sender_only_unsigned(&mut self) -> AppResult<()> { | ||
| self.unsigned.remove("transaction_id"); | ||
| self.unsigned.remove("org.matrix.msc4140.delay_id"); |
There was a problem hiding this comment.
Fresh evidence after the earlier delay-ID leak finding is that the new helper is only called by the timeline/topology/relation loaders: routing/client/room/event.rs:68-71 serializes a directly fetched PDU unchanged, while sync_v3.rs:596-605, sync_v5.rs:820-837, and routing/client/room/state.rs:49-54 do the same for state PDUs. Consequently, another room member can still retrieve org.matrix.msc4140.delay_id through /rooms/{room}/event/{event_id}, /sync state, sliding-sync required state, or /state; invoke this scrubbing in those paths or move it into the common client-event serialization layer.
Useful? React with 👍 / 👎.
| diesel::delete( | ||
| delayed_events::table | ||
| .filter(delayed_events::finalized_at.is_not_null()) | ||
| .filter(delayed_events::finalized_at.le(finalized_before)), |
There was a problem hiding this comment.
When a successfully sent row reaches the retention cutoff, this deletes only its delayed_events record, while send_delayed_pdu has permanently inserted the same transaction into event_idempotents and there is no corresponding cleanup. A later retry of the original scheduling PUT therefore misses get_by_txn_id, receives a new delay_id, and, when due, resolves the stale transaction mapping and finalizes against the old event without appending the newly scheduled content; prune the associated mapping or retain an idempotency tombstone that returns the original outcome.
Useful? React with 👍 / 👎.
`GET /rooms/{roomId}/event/{eventId}` serialized a directly fetched PDU
without any scrubbing, so `transaction_id` reached any room member who
asked for the event by id -- a pre-existing leak that MSC4140's
`delay_id` inherited, since the helper added for it is only wired into
the timeline, topology and relation loaders.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed the scrubbing gap in the latest push. You were right, and it is worse than a delayed-events issue: On "Preserve delayed-event idempotency when pruning": agreed that the @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 468cede1d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Another worker is already sending it, which is the outcome | ||
| // this request wanted. | ||
| if send_in_flight(&refreshed, now) { | ||
| return Ok(()); |
There was a problem hiding this comment.
When two send actions overlap, this returns success to the second caller merely because the first holds a lease. If the first send then fails authorization or another pre-append check, it unclaims the row and returns an error while the second caller has already been told the event was sent; the event remains scheduled and may fire later. Return a conflict or wait for the in-flight attempt's actual outcome instead of treating the lease as a successful send.
Useful? React with 👍 / 👎.
| if let Some(state_key) = &event.state_key { | ||
| let state_event_type: StateEventType = event.event_type.clone().into(); | ||
| crate::state::allowed_to_send_state_event( | ||
| &event.room_id, | ||
| &state_event_type, | ||
| state_key, | ||
| &serde_json::from_value(event.content.clone())?, |
There was a problem hiding this comment.
Run the transaction-id recovery lookup before this fallible state validation. If the process crashes after appending a state event and recording its transaction mapping but before set_sent, recovery reruns these checks before consulting the mapping; for example, an appended restricted m.room.member join can now fail because the target is already joined. The scheduler then records an error even though the delayed event was successfully persisted.
Useful? React with 👍 / 👎.
Two more from review: - The transaction-id recovery lookup ran after the state-event authorization check, so recovering a state event that had already reached the room re-ran authorization against state the event itself may have changed -- turning a completed send into a permanent failure. The lookup now runs first. It stays under the room lock, which is simply taken earlier. - A manual `send` that found another worker holding the lease returned success. That was a guess: the other send can still fail a pre-append check and release the lease, leaving the caller told the event was sent when it never was. Report the in-flight conflict instead so a retry gets a definitive answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both fixed in the latest push. The ordering one was a regression from my own fencing change — moving the txn lookup under the room lock also moved it after the state check, and you are right that re-running authorization on recovery can permanently fail a send that already landed. The lock is now taken earlier so the lookup can come first and still be fenced. The premature success on an in-flight @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fedc689b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| delayed_events::send_at.eq(delayed_events::delay_ms + now), | ||
| // Invalidate any expired lease, so a worker that outlived it cannot | ||
| // still record an outcome against the row this restart just rescheduled. | ||
| delayed_events::claimed_at.eq(None::<i64>), |
There was a problem hiding this comment.
When a send remains alive beyond the five-minute lease—for example, while blocked acquiring the room lock—restart clears its claim and reports success, but that worker still holds its previously loaded row and send_delayed_pdu never revalidates the lease before appending, so it can send immediately despite the new timer; cancel has the same race. Fresh evidence in this revision is that clearing claimed_at only prevents the stale worker's later set_sent write, not the timeline append itself. Add a durable fence immediately before the append, or otherwise prevent an expired worker from continuing after either management transition.
Useful? React with 👍 / 👎.
|
This is the seventh round, and the new finding is the same root cause as the last three: a worker that outlives its lease is not fenced, because nothing revalidates the claim between the check and the append. I have fixed each instance as it surfaced, and each fix has surfaced the next one. That pattern is the answer, not a reason to keep going. I am stopping the patch loop here. The mechanism cannot be made correct by more of these changes:
A correct version needs the claim and the append under one fence. The shape that fits this codebase is a What has been fixed across these rounds is real and the branch is materially better than where it started: the delay-id leak (including a pre-existing Worth restating for whoever decides: |
# Conflicts: # Cargo.toml # crates/server/src/routing/client.rs
|
Addressed the remaining correctness and compatibility findings:
Verification was repeated in two clean rounds after the final change: rustfmt/diff checks, Clippy with warnings denied, all 335 palpo-core tests with unstable-msc4140 plus integration/doc tests, server delayed-event tests, and isolated PostgreSQL 18 up/down migration race scenarios. Both rounds used the same final diff hash. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f1a3dafe6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let now = UnixMillis::now().get() as i64; | ||
| let next_wake = match delayed_event::next_send_at().await { | ||
| Ok(send_at) => send_at, |
There was a problem hiding this comment.
With the explicitly supported db.pool_size = 2, a sender holds one connection and its delayed-event row lock while send_delayed_pdu obtains additional pooled connections; a concurrent restart or cancel can take the nominally spare connection and block on that same row, leaving the sender blocked waiting for the pool. The cycle lasts until the pool timeout and causes the send transaction to roll back or return a server error; the same deadlock occurs at larger pool sizes when pool_size - 1 sends run concurrently. Limiting send permits does not reserve the remaining connection, so the append must avoid nested pool acquisition or use a genuinely dedicated connection.
Useful? React with 👍 / 👎.
|
Fixed the CI failures in
After the final change, two clean review rounds passed over the identical diff ( |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0f4e61155
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(super) async fn send_state_for_key( | ||
| _aa: AuthArgs, | ||
| args: StateEventsForKeyReqArgs, | ||
| args: SendStateEventReqArgs, |
There was a problem hiding this comment.
ts for immediate state sends
When an appservice calls either state PUT endpoint with ?ts= but without a delay, the new SendStateEventReqArgs parses and advertises that parameter, but only the delayed branch reads args.timestamp; the immediate branch still invokes send_state_event_for_key, whose PduBuilder receives no timestamp, so origin_server_ts is silently replaced with the current time. Thread the appservice timestamp through both immediate state-event paths as well.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f358638b4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -- same delay id. | ||
| CREATE TABLE delayed_event_outputs ( | ||
| delay_id TEXT NOT NULL PRIMARY KEY, | ||
| event_id TEXT NOT NULL UNIQUE |
There was a problem hiding this comment.
In a multi-instance deployment, two simultaneously due delayed events in the same room can read the same predecessors and produce identical signed fields (especially when an appservice supplies the same ts); Matrix reference hashes omit unsigned, so their different delay and transaction IDs do not prevent the same event ID. DbEventData::save then resolves the second insert as an update, bypassing the AFTER INSERT candidate trigger, while this uniqueness constraint prevents both delay IDs from mapping to that event. Depending on promotion ordering, one delay is left without a durable recovery fence or both promotions fail, allowing a later recovery to append a duplicate or record failed sends; serialize same-room appends across instances or make output tracking handle colliding event IDs for every delay.
Useful? React with 👍 / 👎.
1 participant
Closes #319
Summary
Implements the current MSC4140 delayed-events API with persistent scheduling, matching upstream Ruma's revised API shapes (ruma/ruma@26406af).
Endpoints (all under
/_matrix/client/unstable/org.matrix.msc4140, authed + rate-limited)PUT /rooms/{room_id}/delayed_event/{event_type}/{txn_id}— schedule a message or state event (delayin ms, optionalstate_key,content); returnsdelay_id. Idempotent per session transaction id. Supports appservice timestamp massaging via?ts=.GET /delayed_events— the user's scheduled delayed events in chronological send orderGET /delayed_events/{delay_id}— one delayed event, scheduled or finalized (withevent_id/error/finalised_ts)POST /delayed_events/{delay_id}/{action}—restart/send/cancel, plus the deprecated body-action form (POST /delayed_events/{delay_id}with{"action": ...}) for older clientsSemantics per the MSC
delayed_eventstable; on startup the scheduler sends overdue events in chronological order of their scheduled send timesbuild_and_append_pduauthorization/state/federation paths; power levels are evaluated when the delay elapses, not at schedulingsend/cancelcannot double-send. Actions are idempotent on matching outcomes and return HTTP 409 on conflicting ones; unknowndelay_id→ 404M_NOT_FOUNDerror(no retry); a failed manualsendkeeps the event scheduled so the client can retrymax_delay_ms(403M_FORBIDDEN) and per-usermax_scheduled(429M_LIMIT_EXCEEDEDwithRetry-Afterheader andretry_after_ms); finalized events pruned after a retention periodorg.matrix.msc4140in/versionsand theorg.matrix.msc4140.delayed_eventslimits capability are only exposed when enabled; disabled deployments register no routesunsignedgainsorg.matrix.msc4140.delay_idandtransaction_idon the resulting eventConfig
New
[delayed_events]section:enable(default true),max_delay_ms(24 h),max_scheduled(100),retention_ms(7 days).palpo-example.tomlregenerated.Notes
M_CONCURRENT_WRITE(ruma/ruma@66eeed5); that error code belongs to MSC4388 rendezvous sessions, not the delayed-events API, so it is not required here — conflicting management actions use HTTP 409 withM_UNKNOWNas the MSC specifies?org.matrix.msc4140.delay=query-parameter variant on/sendand/state(what Synapse currently implements) is left as a possible follow-upRetryAfteris now publicly exported from palpo-core andM_LIMIT_EXCEEDEDresponses now emit theRetry-Afterheader /retry_after_msbody fieldTesting
cargo test --all --all-features(new serde/status tests inpalpo-core::client::delayed_events)cargo +nightly fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warnings🤖 Generated with Claude Code