feat: Add application service (bridges) support#394
Open
sampaiodiego wants to merge 79 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a new ChangesApplication Service Bridge Integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EduService
participant EventRouterService
participant NamespaceMatcherService
participant TransactionSenderService
participant Bridge
EduService->>EventRouterService: routeEphemeral(typing|presence|receipt EDU)
EventRouterService->>NamespaceMatcherService: getInterestedAppServices(...)
NamespaceMatcherService-->>EventRouterService: interested appservices
EventRouterService->>TransactionSenderService: sendTransaction(appservice, batch)
TransactionSenderService->>Bridge: PUT /_matrix/app/v1/transactions/{txnId}
Bridge-->>TransactionSenderService: response
sequenceDiagram
participant Client
participant HomeserverController
participant resolveAppServiceAuth
participant FederationSDK
Client->>HomeserverController: appservice-authenticated request
HomeserverController->>resolveAppServiceAuth: resolveAppServiceAuth(...)
resolveAppServiceAuth->>FederationSDK: getRegistrationByAsToken(token)
FederationSDK-->>resolveAppServiceAuth: registration
resolveAppServiceAuth-->>HomeserverController: auth result or error
HomeserverController-->>Client: Matrix response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 50.64% 51.06% +0.41%
==========================================
Files 101 113 +12
Lines 11533 12708 +1175
==========================================
+ Hits 5841 6489 +648
- Misses 5692 6219 +527 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
add-application-service-support
branch
from
1fa0373 to
4ee3063
Compare
…ing batch handling
… handling in repository and service
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/appservice/src/services/event-router.service.ts">
<violation number="1" location="packages/appservice/src/services/event-router.service.ts:41">
P2: Removing the `routeEvent` wrapper breaks the mock in `packages/federation-sdk/src/services/event-sender.service.spec.ts`, which still expects `routeEvent` on `EventRouterService`. The actual `EventSenderService` code now calls `routePersistent`, so that test will throw a runtime error when it tries to invoke the missing method on the mock. Please update the test to mock and assert `routePersistent` instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/appservice/src/services/transaction-sender.service.ts (1)
236-245: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTransport failures are logged/recorded as
HTTP undefined.When
fetch()fails at the transport layer it resolves withok: falseandstatus: undefined(it does not throw), so this branch — not thecatch— runs, loggingstatus: undefinedand callingmarkDown(..., 'HTTP undefined'). The down-marking is correct, but the reason is misleading for the common connection-failure case. Consider distinguishingresponse.status === undefined(connection failed) from a genuine non-2xx, mirroringping.service.ts.🪵 Suggested distinction
- this.logger.warn({ - msg: `Transaction delivery failed`, - asId: registration._id, - txnId, - status: response.status, - }); - - await this.txnRepo.markFailed(registration._id, txnId); - await this.stateRepo.markDown(registration._id, `HTTP ${response.status}`); - return false; + const reason = response.status === undefined ? 'Failed to connect to appservice' : `HTTP ${response.status}`; + this.logger.warn({ msg: 'Transaction delivery failed', asId: registration._id, txnId, status: response.status, reason }); + + await this.txnRepo.markFailed(registration._id, txnId); + await this.stateRepo.markDown(registration._id, reason); + return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/appservice/src/services/transaction-sender.service.ts` around lines 236 - 245, The failure handling in transaction-sender.service.ts logs and marks transport errors as HTTP undefined, which is misleading when fetch() returns a response with status unset. Update the non-ok response branch around the logger.warn, txnRepo.markFailed, and stateRepo.markDown calls to distinguish response.status === undefined from a real HTTP status, similar to ping.service.ts. Use a clearer reason string for connection/transport failures while keeping genuine non-2xx responses recorded with their actual HTTP status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/appservice/src/services/transaction-sender.service.ts`:
- Around line 236-245: The failure handling in transaction-sender.service.ts
logs and marks transport errors as HTTP undefined, which is misleading when
fetch() returns a response with status unset. Update the non-ok response branch
around the logger.warn, txnRepo.markFailed, and stateRepo.markDown calls to
distinguish response.status === undefined from a real HTTP status, similar to
ping.service.ts. Use a clearer reason string for connection/transport failures
while keeping genuine non-2xx responses recorded with their actual HTTP status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d5d88df-77c2-4d0a-b4cd-cc7a1ff75e11
📒 Files selected for processing (21)
docs/bridge-architecture-guide.mdpackages/appservice/src/index.tspackages/appservice/src/models/appservice.model.tspackages/appservice/src/repositories/appservice-state.repository.tspackages/appservice/src/repositories/appservice-txn.repository.tspackages/appservice/src/services/event-router.service.spec.tspackages/appservice/src/services/event-router.service.tspackages/appservice/src/services/namespace-matcher.service.spec.tspackages/appservice/src/services/namespace-matcher.service.tspackages/appservice/src/services/ping.service.tspackages/appservice/src/services/registration.service.tspackages/appservice/src/services/transaction-sender.service.tspackages/core/src/utils/fetch.tspackages/federation-sdk/src/index.tspackages/federation-sdk/src/repositories/room-alias.repository.tspackages/federation-sdk/src/services/directory.service.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event-sender.service.spec.tspackages/federation-sdk/src/services/event-sender.service.tspackages/federation-sdk/src/services/message.service.tspackages/federation-sdk/src/services/room.service.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/appservice/src/index.ts
- packages/federation-sdk/src/services/directory.service.ts
- packages/appservice/src/repositories/appservice-txn.repository.ts
- packages/appservice/src/services/event-router.service.spec.ts
- packages/appservice/src/models/appservice.model.ts
- packages/federation-sdk/src/services/edu.service.ts
- packages/federation-sdk/src/services/event-sender.service.spec.ts
- packages/appservice/src/repositories/appservice-state.repository.ts
- packages/appservice/src/services/registration.service.ts
- packages/federation-sdk/src/services/event-sender.service.ts
- packages/appservice/src/services/namespace-matcher.service.spec.ts
- packages/appservice/src/services/namespace-matcher.service.ts
- packages/federation-sdk/src/services/room.service.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Quality Checks(lint, test, tsc)
🔇 Additional comments (8)
packages/appservice/src/services/ping.service.ts (1)
52-74: LGTM!packages/federation-sdk/src/repositories/room-alias.repository.ts (1)
25-35: LGTM!docs/bridge-architecture-guide.md (1)
115-131: LGTM!packages/federation-sdk/src/index.ts (1)
156-192: LGTM!packages/core/src/utils/fetch.ts (1)
123-184: LGTM!Also applies to: 233-248
packages/appservice/src/services/event-router.service.ts (1)
30-68: LGTM!packages/appservice/src/services/transaction-sender.service.ts (1)
60-87: LGTM!Also applies to: 149-200
packages/federation-sdk/src/services/message.service.ts (1)
54-54: LGTM!Also applies to: 128-128, 182-182, 262-262, 289-289, 334-334, 372-372
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/index.ts">
<violation number="1" location="packages/federation-sdk/src/index.ts:192">
P2: `init()` starts a background retry scheduler via `transactionSender.startRetryScheduler()`, but there is no corresponding teardown path exposed. The underlying `setInterval` in `TransactionSenderService` is not `.unref()`'d, so it will keep the Node process alive indefinitely. In tests, workers, or short-lived scripts this prevents clean exit, and the existing `stopRetryScheduler()` method is never called. Consider either exposing a shutdown hook from `init()` that calls `stopRetryScheduler()`, or `.unref()`'ing the timer so it does not block process exit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
6 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/sdk.ts">
<violation number="1" location="packages/federation-sdk/src/sdk.ts:312">
P1: Recovery is entirely process-local (`recoverers` is an in-memory `Map`) while the DOWN state and transaction backlog live in shared MongoDB. This means if the instance that marked a bridge DOWN exits before its local `setTimeout` recoverer fires, no other instance will pick up the shared backlog — `sendTransaction` simply queues new events without starting a recoverer, and `startRecoverersForDownServices` is only invoked at `setConfig` time. In a multi-instance deployment the bridge can stay permanently stuck in DOWN with undelivered transactions until a config reload or restart. Consider adding a lightweight periodic background poller or a state-change watcher so another instance can adopt recovery of bridges that are DOWN but have no live recoverer.</violation>
</file>
<file name="packages/appservice/src/services/ping.service.ts">
<violation number="1" location="packages/appservice/src/services/ping.service.ts:59">
P2: The `/timed out/i` regex only catches the inactivity timeout from the fetch wrapper (`'Request timed out after 20s'`) and misses the Node.js transport-level `ETIMEDOUT` errors (e.g., `'connect ETIMEDOUT ...'`). Those genuine connection timeouts will be returned as `M_CONNECTION_FAILED` even though the type union advertises a dedicated `M_CONNECTION_TIMEOUT` code. Broaden the check so transport-level timeouts are also classified correctly — for example by also matching `ETIMEDOUT` or `\btimeout\b` in the reason string.</violation>
</file>
<file name="packages/appservice/src/services/transaction-sender.service.ts">
<violation number="1" location="packages/appservice/src/services/transaction-sender.service.ts:101">
P2: Delivery failures no longer preserve or update the persisted appservice error cause and timestamp after the first failure.
`putTransaction` returns `false` for both HTTP and transport exceptions without calling `markDown`, so the specific HTTP status or exception message that previously reached the state record is lost. The first failure is instead stored as the generic message `Failed to deliver transaction ${txnId}` via `startRecoverer`. Subsequent retry failures in `retry`/`backoffAndReschedule` only reschedule and never call `markDown` again, so `lastError` and `lastErrorAt` remain stuck at the initial outage time even as the bridge continues to fail.
Since `AppServiceState` fields are exposed through the state repository, operators and health consumers receive stale, low-information diagnostics. Consider updating `lastError`/`lastErrorAt` on each retry failure (e.g. in `putTransaction` or `backoffAndReschedule`) so the persisted state reflects the most recent actionable cause.</violation>
<violation number="2" location="packages/appservice/src/services/transaction-sender.service.ts:128">
P1: Runtime bridge configuration changes do not update or cancel already-running recoverers. `RegistrationService.initialize()` rebuilds the registration cache (and is documented to be safe for repeated calls, e.g. after `setConfig`), but `TransactionSenderService.startRecoverersForDownServices` skips any `asId` that already has a recoverer. This means:
- Changing the bridge URL or rotating `hsToken` while the bridge is down leaves the recoverer delivering queued backlog events to the old URL with the old credentials.
- Removing the bridge config entirely still leaves the recoverer alive; when its timer fires and finds an empty queue, it calls `markUp` and recreates state that `initialize` had just cleaned up.
Consider adding a method to `TransactionSenderService` that reconciles or cancels recoverers when registrations are rebuilt, or refresh the `CachedAppService` reference inside the recoverer before each retry.</violation>
<violation number="3" location="packages/appservice/src/services/transaction-sender.service.ts:165">
P1: A transient database error can permanently stall an appservice recoverer. The `scheduleRetry` timer callback only logs errors escaping `retry()`; it does not reschedule. Inside `retry()`, MongoDB operations (`getOldestPending`, `complete`, `markUp`) are not individually protected, so any thrown error kills the recoverer with no retry scheduled. You could close the gap by rescheduling in the `.catch()` handler, or by wrapping the body of `retry()` in a catch-all that calls `backoffAndReschedule`.</violation>
<violation number="4" location="packages/appservice/src/services/transaction-sender.service.ts:193">
P1: A TOCTOU race condition between `getOldestPending` returning null and `markUp` committing can permanently strand a transaction. If a concurrent `sendTransaction` runs after `recoverers.delete(asId)` but before `markUp` completes, it still sees the bridge as `down`, persists the transaction, and returns without inline delivery. Once `markUp` finishes the bridge is UP with no recoverer running, and `sendTransaction` only ever delivers its own transaction — it never drains backlog — so the orphaned row remains in MongoDB forever. Consider making the empty-queue/mark-up transition atomic, or doing another `getOldestPending` check after `markUp` before returning, so that any transaction inserted during the window gets picked up.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Registrations only exist after the config is applied, so this is the | ||
| // boot hook for resuming recovery of bridges persisted as down. Idempotent | ||
| // (per-bridge recoverer guard), so repeated setConfig calls are safe. | ||
| await this.transactionSenderService.startRecoverersForDownServices(this.registrationService.getAll()); |
There was a problem hiding this comment.
P1: Recovery is entirely process-local (recoverers is an in-memory Map) while the DOWN state and transaction backlog live in shared MongoDB. This means if the instance that marked a bridge DOWN exits before its local setTimeout recoverer fires, no other instance will pick up the shared backlog — sendTransaction simply queues new events without starting a recoverer, and startRecoverersForDownServices is only invoked at setConfig time. In a multi-instance deployment the bridge can stay permanently stuck in DOWN with undelivered transactions until a config reload or restart. Consider adding a lightweight periodic background poller or a state-change watcher so another instance can adopt recovery of bridges that are DOWN but have no live recoverer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/sdk.ts, line 312:
<comment>Recovery is entirely process-local (`recoverers` is an in-memory `Map`) while the DOWN state and transaction backlog live in shared MongoDB. This means if the instance that marked a bridge DOWN exits before its local `setTimeout` recoverer fires, no other instance will pick up the shared backlog — `sendTransaction` simply queues new events without starting a recoverer, and `startRecoverersForDownServices` is only invoked at `setConfig` time. In a multi-instance deployment the bridge can stay permanently stuck in DOWN with undelivered transactions until a config reload or restart. Consider adding a lightweight periodic background poller or a state-change watcher so another instance can adopt recovery of bridges that are DOWN but have no live recoverer.</comment>
<file context>
@@ -304,6 +306,10 @@ export class FederationSDK {
+ // Registrations only exist after the config is applied, so this is the
+ // boot hook for resuming recovery of bridges persisted as down. Idempotent
+ // (per-bridge recoverer guard), so repeated setConfig calls are safe.
+ await this.transactionSenderService.startRecoverersForDownServices(this.registrationService.getAll());
}
</file context>
| * Boot path: resume recovery for bridges persisted as DOWN. Bridges | ||
| * persisted as UP are left alone (matches Synapse). | ||
| */ | ||
| async startRecoverersForDownServices(appservices: CachedAppService[]): Promise<void> { |
There was a problem hiding this comment.
P1: Runtime bridge configuration changes do not update or cancel already-running recoverers. RegistrationService.initialize() rebuilds the registration cache (and is documented to be safe for repeated calls, e.g. after setConfig), but TransactionSenderService.startRecoverersForDownServices skips any asId that already has a recoverer. This means:
- Changing the bridge URL or rotating
hsTokenwhile the bridge is down leaves the recoverer delivering queued backlog events to the old URL with the old credentials. - Removing the bridge config entirely still leaves the recoverer alive; when its timer fires and finds an empty queue, it calls
markUpand recreates state thatinitializehad just cleaned up.
Consider adding a method to TransactionSenderService that reconciles or cancels recoverers when registrations are rebuilt, or refresh the CachedAppService reference inside the recoverer before each retry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/appservice/src/services/transaction-sender.service.ts, line 128:
<comment>Runtime bridge configuration changes do not update or cancel already-running recoverers. `RegistrationService.initialize()` rebuilds the registration cache (and is documented to be safe for repeated calls, e.g. after `setConfig`), but `TransactionSenderService.startRecoverersForDownServices` skips any `asId` that already has a recoverer. This means:
- Changing the bridge URL or rotating `hsToken` while the bridge is down leaves the recoverer delivering queued backlog events to the old URL with the old credentials.
- Removing the bridge config entirely still leaves the recoverer alive; when its timer fires and finds an empty queue, it calls `markUp` and recreates state that `initialize` had just cleaned up.
Consider adding a method to `TransactionSenderService` that reconciles or cancels recoverers when registrations are rebuilt, or refresh the `CachedAppService` reference inside the recoverer before each retry.</comment>
<file context>
@@ -69,138 +84,168 @@ export class TransactionSenderService {
- private async pollAppService(appservice: CachedAppService): Promise<void> {
- const asId = appservice.registration._id;
- const state = await this.stateRepo.getState(asId);
+ async startRecoverersForDownServices(appservices: CachedAppService[]): Promise<void> {
+ for (const appservice of appservices) {
+ const asId = appservice.registration._id;
</file context>
| for (;;) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const txn = await this.txnRepo.getOldestPending(asId); | ||
| if (!txn) { |
There was a problem hiding this comment.
P1: A TOCTOU race condition between getOldestPending returning null and markUp committing can permanently strand a transaction. If a concurrent sendTransaction runs after recoverers.delete(asId) but before markUp completes, it still sees the bridge as down, persists the transaction, and returns without inline delivery. Once markUp finishes the bridge is UP with no recoverer running, and sendTransaction only ever delivers its own transaction — it never drains backlog — so the orphaned row remains in MongoDB forever. Consider making the empty-queue/mark-up transition atomic, or doing another getOldestPending check after markUp before returning, so that any transaction inserted during the window gets picked up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/appservice/src/services/transaction-sender.service.ts, line 193:
<comment>A TOCTOU race condition between `getOldestPending` returning null and `markUp` committing can permanently strand a transaction. If a concurrent `sendTransaction` runs after `recoverers.delete(asId)` but before `markUp` completes, it still sees the bridge as `down`, persists the transaction, and returns without inline delivery. Once `markUp` finishes the bridge is UP with no recoverer running, and `sendTransaction` only ever delivers its own transaction — it never drains backlog — so the orphaned row remains in MongoDB forever. Consider making the empty-queue/mark-up transition atomic, or doing another `getOldestPending` check after `markUp` before returning, so that any transaction inserted during the window gets picked up.</comment>
<file context>
@@ -69,138 +84,168 @@ export class TransactionSenderService {
+ for (;;) {
+ // eslint-disable-next-line no-await-in-loop
+ const txn = await this.txnRepo.getOldestPending(asId);
+ if (!txn) {
+ this.recoverers.delete(asId);
// eslint-disable-next-line no-await-in-loop
</file context>
|
|
||
| private scheduleRetry(recoverer: Recoverer): void { | ||
| const delayMs = 2 ** recoverer.backoffCounter * 1000; | ||
| recoverer.timer = setTimeout(() => { |
There was a problem hiding this comment.
P1: A transient database error can permanently stall an appservice recoverer. The scheduleRetry timer callback only logs errors escaping retry(); it does not reschedule. Inside retry(), MongoDB operations (getOldestPending, complete, markUp) are not individually protected, so any thrown error kills the recoverer with no retry scheduled. You could close the gap by rescheduling in the .catch() handler, or by wrapping the body of retry() in a catch-all that calls backoffAndReschedule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/appservice/src/services/transaction-sender.service.ts, line 165:
<comment>A transient database error can permanently stall an appservice recoverer. The `scheduleRetry` timer callback only logs errors escaping `retry()`; it does not reschedule. Inside `retry()`, MongoDB operations (`getOldestPending`, `complete`, `markUp`) are not individually protected, so any thrown error kills the recoverer with no retry scheduled. You could close the gap by rescheduling in the `.catch()` handler, or by wrapping the body of `retry()` in a catch-all that calls `backoffAndReschedule`.</comment>
<file context>
@@ -69,138 +84,168 @@ export class TransactionSenderService {
+
+ private scheduleRetry(recoverer: Recoverer): void {
+ const delayMs = 2 ** recoverer.backoffCounter * 1000;
+ recoverer.timer = setTimeout(() => {
+ recoverer.timer = null;
+ void this.retry(recoverer).catch((err) => {
</file context>
| // with status undefined, and the failure reason is only retrievable | ||
| // through the rejecting body accessors (see errorResponse in utils/fetch.ts). | ||
| if (response.status === undefined) { | ||
| const reason = await response.text().catch((r) => String(r)); |
There was a problem hiding this comment.
P2: The /timed out/i regex only catches the inactivity timeout from the fetch wrapper ('Request timed out after 20s') and misses the Node.js transport-level ETIMEDOUT errors (e.g., 'connect ETIMEDOUT ...'). Those genuine connection timeouts will be returned as M_CONNECTION_FAILED even though the type union advertises a dedicated M_CONNECTION_TIMEOUT code. Broaden the check so transport-level timeouts are also classified correctly — for example by also matching ETIMEDOUT or \btimeout\b in the reason string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/appservice/src/services/ping.service.ts, line 59:
<comment>The `/timed out/i` regex only catches the inactivity timeout from the fetch wrapper (`'Request timed out after 20s'`) and misses the Node.js transport-level `ETIMEDOUT` errors (e.g., `'connect ETIMEDOUT ...'`). Those genuine connection timeouts will be returned as `M_CONNECTION_FAILED` even though the type union advertises a dedicated `M_CONNECTION_TIMEOUT` code. Broaden the check so transport-level timeouts are also classified correctly — for example by also matching `ETIMEDOUT` or `\btimeout\b` in the reason string.</comment>
<file context>
@@ -50,18 +53,22 @@ export class PingService {
- errcode: 'M_CONNECTION_FAILED',
- error: 'Failed to connect to appservice',
- };
+ const reason = await response.text().catch((r) => String(r));
+ if (/timed out/i.test(reason)) {
+ return { errcode: 'M_CONNECTION_TIMEOUT', error: reason };
</file context>
| } | ||
|
|
||
| // The row stays as the head of the backlog; its ephemeral riders are lost. | ||
| await this.startRecoverer(appservice, `Failed to deliver transaction ${txnId}`); |
There was a problem hiding this comment.
P2: Delivery failures no longer preserve or update the persisted appservice error cause and timestamp after the first failure.
putTransaction returns false for both HTTP and transport exceptions without calling markDown, so the specific HTTP status or exception message that previously reached the state record is lost. The first failure is instead stored as the generic message Failed to deliver transaction ${txnId} via startRecoverer. Subsequent retry failures in retry/backoffAndReschedule only reschedule and never call markDown again, so lastError and lastErrorAt remain stuck at the initial outage time even as the bridge continues to fail.
Since AppServiceState fields are exposed through the state repository, operators and health consumers receive stale, low-information diagnostics. Consider updating lastError/lastErrorAt on each retry failure (e.g. in putTransaction or backoffAndReschedule) so the persisted state reflects the most recent actionable cause.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/appservice/src/services/transaction-sender.service.ts, line 101:
<comment>Delivery failures no longer preserve or update the persisted appservice error cause and timestamp after the first failure.
`putTransaction` returns `false` for both HTTP and transport exceptions without calling `markDown`, so the specific HTTP status or exception message that previously reached the state record is lost. The first failure is instead stored as the generic message `Failed to deliver transaction ${txnId}` via `startRecoverer`. Subsequent retry failures in `retry`/`backoffAndReschedule` only reschedule and never call `markDown` again, so `lastError` and `lastErrorAt` remain stuck at the initial outage time even as the bridge continues to fail.
Since `AppServiceState` fields are exposed through the state repository, operators and health consumers receive stale, low-information diagnostics. Consider updating `lastError`/`lastErrorAt` on each retry failure (e.g. in `putTransaction` or `backoffAndReschedule`) so the persisted state reflects the most recent actionable cause.</comment>
<file context>
@@ -69,138 +84,168 @@ export class TransactionSenderService {
+ }
+
+ // The row stays as the head of the backlog; its ephemeral riders are lost.
+ await this.startRecoverer(appservice, `Failed to deliver transaction ${txnId}`);
}
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit