Skip to content

Commit 3e081f6

Browse files
kodes-agencyclaude
andauthored
fix(sync): count a Property with a recorded failure in the reconciliation-age alert (#1082)
sync.reconciliation-age left out every Property with a recorded sync failure, so a Property whose full reads all failed could not raise the alert that watches for a Property with no completed read. The read now judges every syncable Property alike. A connection that needs reauthorization, including one revoked by the provider, is not syncable and stays out: its reconnect notice is its signal. Runbook, alerts table and ADR 0055 are updated. Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
1 parent ab7b2ed commit 3e081f6

6 files changed

Lines changed: 84 additions & 26 deletions

‎docs/adr/0055-stable-review-and-inbox-handling-cycles.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,6 +1148,10 @@ control just inside the limit).
11481148

11491149
A reply observation's `content_expires_at` was written only when the row was inserted, and a read that finds the same reply records no row. So a reply Google kept showing stopped being served 30 days after it was first stored, while its review's clock moved on every fetch. A restating read now moves the head row's `content_expires_at` to that read's expiry and its `updated_at` to the read's time, forward only and only while the row holds a reply (`renewHeadContentClock`). It writes no row, revision or fact, and leaves `observed_at` and `provider_updated_at` alone. The clock counts from the last fetch that returned the reply, as ADR 0031 requires for a review's content.
11501150

1151+
## Note 2026-11-04 (b) — the reconciliation-age alert counts a Property with a recorded failure
1152+
1153+
`sync.reconciliation-age` left out any Property with an `error_class`, because `sync.failed-nonzero` already names a coded failure. That alert raises only when a retry date passes before the next look re-dates the row, so a Property that failed every full read for days raised no standing signal at all. The read now judges every syncable Property alike. A Property whose connection needs reauthorization (`reauth_required`, which is also how `provider_revoked` is recorded) is not syncable, so it is still left to the reconnect notice.
1154+
11511155
## Merged from ADR 0003
11521156

11531157
Introduce a new `review` bounded context for external platform reviews,

‎docs/operations/alerts-and-security-posture.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ Every alert is defined in `src/shared/observability/alert-definitions.ts` (owner
153153
| `sync.failed-nonzero` | P2 | any coded sync failure whose retry is due | §13 |
154154
| `sync.push-missed` | P2 | topic set; any review polling found in 24h, dated within a day of that, with no Google push accepted within 60min | §4 |
155155
| `sync.push-silent` | P2 | topic set; polling found new reviews in 24h and Pub/Sub accepted no push at all in that time | §4 |
156-
| `sync.reconciliation-age` | P2 | a syncable Property's newest completed reconciliation is over 8h old (parked coded failures excluded; independent of push) | §13 |
156+
| `sync.reconciliation-age` | P2 | a syncable Property's newest completed reconciliation is over 8h old (counts failed Properties; independent of push) | §13 |
157157
| `ai.review-analysis-stalled` | P2 | incomplete enabled coverage with no new settlement for > 15min; monitor failure also fires | §22 |
158158
| `ai.review-analysis-empty-enrollment` | P2 | any queued/running zero-snapshot enrollment while eligible reviews exist | §22 |
159159
| `retention.failure` | P1 | latest retention run failed for any subject; monitor failure (the `retention_runs` read) also fires | §8 |

‎docs/operations/runbooks.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ SELECT listing_complete, count(*), max(evaluated_at)
300300

301301
**What the system does.** The snapshot run ends as `provider_rejected` (`review_provider_snapshot_runs.failure_code`) and discards its cursors. The worker writes one warning, `review_snapshot_provider_rejected`, with `providerStatus` (Google's HTTP status, 400 or 404; null if the error carried none), `dispatch` (`answered`) and `runId`. Look the status up in the worker log: 404 means Google no longer has the Location through this connection, 400 means it refuses the request for it. The job then parks the Property like a location over the cap: it records `error_class = provider_rejected`, the failure time and the retry date, logs `review_snapshot_failure_deferred`, and ends normally. It throws nothing, so there is no BullMQ retry, no dead letter, no quarantine entry, no Sentry event and no repair latch (`worker.job-runtime-unready` stays clear), and `markSyncSucceeded` is not called. The wait is the ordinary ladder (15 minutes, 1 hour, 4 hours, 6 hours cap), not a day, so a Location fixed on Google is read again within hours and a completed run clears the failure.
302302

303-
**What you see.** Only `sync.failed-nonzero`, with `alert_codes` `provider_rejected:N` (N Properties), and only as an edge, not a standing signal. The retry date is written into the future when the failure is recorded, and the alert counts a Property only from that date until the next look fails again and dates a new one: about one 15-minute sweep per rung of the ladder, once per 6 hours at the cap. So it raises and clears on every rung while the Location stays unreachable, and a quiet alert does not mean the Location is readable. The durable view of what is rejected right now is the query under "What to do". 401 and 403 are not this: they are `authorization_changed` / `authorization_denied` (reconnect Google), 429 and 5xx are retried on the same run, and a code-less failure is still `provider_failure` with its loud error line.
303+
**What you see.** `sync.failed-nonzero`, with `alert_codes` `provider_rejected:N` (N Properties), only as an edge, not a standing signal; and `sync.reconciliation-age` (§13), which is standing, once the Property has gone 8 hours without a completed reconciliation. The retry date is written into the future when the failure is recorded, and `sync.failed-nonzero` counts a Property only from that date until the next look fails again and dates a new one: about one 15-minute sweep per rung of the ladder, once per 6 hours at the cap. So it raises and clears on every rung while the Location stays unreachable, and a quiet one does not mean the Location is readable. The durable view of what is rejected right now is the query under "What to do". 401 and 403 are not this: they are `authorization_changed` / `authorization_denied` (reconnect Google), 429 and 5xx are retried on the same run, and a code-less failure is still `provider_failure` with its loud error line.
304304

305305
**What to do.** Find the Properties: `select property_id, error_class, last_terminal_error_at, error_retry_at from review_sync_state where error_class = 'provider_rejected'` (read-only session). Then check each Location in Google Business Profile: it is still there, verified, not suspended, and still managed by the account the Organization connected (a Location the account no longer manages is the likeliest 404). If the owner removed the Location on purpose, archive or disconnect the Property in RepKey. If it was fixed on Google, nothing else is needed: the next look reads it and clears the failure. To read it sooner, set `next_incremental_at` and `error_retry_at` of that `review_sync_state` row to now and leave `error_class` in place (see "Retry sooner" above). If every location of one connection is rejected, or the status is 400 for locations that look healthy on Google, treat it as a RepKey request defect instead (the status in the warning and the number of Properties tell the two apart) and take it to the engineers; do not wait on Google.
306306

@@ -324,7 +324,7 @@ It enqueues no sync. For each Property with a review whose content expires withi
324324

325325
**Alert:** `sync.reconciliation-age` (P2) — a Property the sweep still polls has no completed reconciliation (a completed provider snapshot run, recorded as a `listing_complete` `review_google_reputation_snapshot_facts` row for its current source epoch; a head fact from `REVIEW_HEAD_CHECKS=persist` does not count) inside the last 8 hours.
326326

327-
**What it means:** the six-hour reconciliation is the safety net that finds a review whose push never came (ADR 0055 decision 5), and the only path while push is dark. The sweep guarantees one about every six hours; the alert fires two hours later, so a slow sweep does not page. A Property with no completed reconciliation yet is counted from the day it became syncable (`properties.updated_at`, `google_connections.credential_authorized_at`, as for `sync.sweep-lag`), and a Property restored with an old reconciliation is counted from the day it qualified again. A Property parked with a coded failure (`review_sync_state.error_class`, for example `review_cap_exceeded` for a location over the cap) is left out: it has its own alert, `sync.failed-nonzero`. The alert does not depend on push. It is held while the `health.sync` or `health.syncPush` section is degraded.
327+
**What it means:** the six-hour reconciliation is the safety net that finds a review whose push never came (ADR 0055 decision 5), and the only path while push is dark. The sweep guarantees one about every six hours; the alert fires two hours later, so a slow sweep does not page. A Property with no completed reconciliation yet is counted from the day it became syncable (`properties.updated_at`, `google_connections.credential_authorized_at`, as for `sync.sweep-lag`), and a Property restored with an old reconciliation is counted from the day it qualified again. A Property with a recorded failure (`review_sync_state.error_class`, for example `provider_rejected`, or `review_cap_exceeded` for a location over the cap) is counted like any other, because a run that fails never completes a reconciliation: `sync.failed-nonzero` names the code, this alert says how long it has been. A Property whose Google connection needs reauthorization (`reauth_required`, which is also how `provider_revoked` is recorded) is not syncable and is not counted: the reconnect notice is its signal. The alert does not depend on push. It is held while the `health.sync` or `health.syncPush` section is degraded.
328328

329329
**First things to check:** `GET /api/health/metrics` → `syncPush.reconciliation.overdueCount` and `oldestAgeMs` (`sync.reconciliation_overdue`, `sync.oldest_reconciliation_age_ms`). If `sync.oldest_due_age_ms` is also high the sweep itself is behind: follow the checks above. If the sweep is healthy, read the overdue Properties' runs: a run that keeps failing before it completes (see the sync failure codes on `review_sync_state`), or a location that never finishes its listing, never writes a fact.
330330

‎src/shared/db/sync-push-health.integration.test.ts‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -390,14 +390,17 @@ describe('reconciliation age read (real reads)', () => {
390390
expect((await read()).reconciliation.overdueCount).toBe(baseline.overdueCount)
391391
})
392392

393-
it('leaves a Property parked with a coded failure to that failure alert', async () => {
394-
const baseline = (await read()).reconciliation
395-
const parked = await seedProperty({ suffix: '17' })
396-
await seedFact(parked, 0, 20 * HOUR)
397-
await seedSyncState(parked, 'review_cap_exceeded')
398-
399-
expect((await read()).reconciliation.overdueCount).toBe(baseline.overdueCount)
400-
})
393+
it.each(['provider_failure', 'provider_rejected', 'review_cap_exceeded'])(
394+
'counts an overdue Property with a recorded %s failure',
395+
async (errorClass) => {
396+
const baseline = (await read()).reconciliation
397+
const parked = await seedProperty({ suffix: '17' })
398+
await seedFact(parked, 0, 20 * HOUR)
399+
await seedSyncState(parked, errorClass)
400+
401+
expect((await read()).reconciliation.overdueCount).toBe(baseline.overdueCount + 1)
402+
},
403+
)
401404

402405
it('judges a Property whose sync state row has no failure like one with no row', async () => {
403406
const baseline = (await read()).reconciliation
@@ -421,6 +424,24 @@ describe('reconciliation age read (real reads)', () => {
421424
expect((await read()).reconciliation.overdueCount).toBe(baseline.overdueCount)
422425
})
423426

427+
it.each([null, 'provider_revoked'])(
428+
'does not double-alert a reauth_required connection (%s)',
429+
async (reason) => {
430+
const baseline = (await read()).reconciliation
431+
const property = await seedProperty({
432+
suffix: '1b',
433+
connectionId: INACTIVE_CONNECTION,
434+
})
435+
await db.execute(
436+
sql`UPDATE google_connections SET status_reason = ${reason} WHERE id = ${INACTIVE_CONNECTION}`,
437+
)
438+
await seedFact(property, 0, 20 * HOUR)
439+
await seedSyncState(property, 'provider_rejected')
440+
441+
expect((await read()).reconciliation.overdueCount).toBe(baseline.overdueCount)
442+
},
443+
)
444+
424445
it('still reports reconciliation age while push is not configured', async () => {
425446
const baseline = (await read(false)).reconciliation
426447
const stale = await seedProperty({ suffix: '1b' })
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import type { SQL } from 'drizzle-orm'
2+
import { PgDialect } from 'drizzle-orm/pg-core'
3+
import { describe, expect, it, vi } from 'vitest'
4+
import type { Database } from '#/shared/db'
5+
import { readSyncPushHealth } from './sync-push-health'
6+
7+
async function reconciliationQuery() {
8+
const where = vi.fn(async (_predicate: SQL | undefined) => [
9+
{ overdue: 1, oldestAgeMs: 36_000_000 },
10+
])
11+
const query = {
12+
from: () => query,
13+
innerJoin: () => query,
14+
leftJoin: () => query,
15+
where,
16+
}
17+
const db = { select: () => query } as unknown as Database
18+
const health = await readSyncPushHealth(db, false)
19+
const predicate = where.mock.calls[0]?.[0]
20+
if (!predicate) throw new Error('Missing reconciliation predicate')
21+
return { health, ...new PgDialect().sqlToQuery(predicate) }
22+
}
23+
24+
describe('reconciliation age selection', () => {
25+
it('does not exclude failed properties from the overdue count', async () => {
26+
const query = await reconciliationQuery()
27+
28+
expect(query.sql).not.toContain('error_class')
29+
expect(query.health.reconciliation).toEqual({
30+
overdueCount: 1,
31+
oldestAgeMs: 36_000_000,
32+
})
33+
})
34+
35+
it('still requires an active connection, which leaves out reauth_required and provider_revoked', async () => {
36+
const query = await reconciliationQuery()
37+
38+
// Revocation is recorded as reauth_required with status_reason provider_revoked.
39+
expect(query.sql).toContain('"google_connections"."status" =')
40+
expect(query.sql).toContain('"google_connections"."credential_use_state" =')
41+
})
42+
})

‎src/shared/db/sync-push-health.ts‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,14 @@
2020
// Review. Only Properties the discovery sweep still polls are judged
2121
// (propertyIsSyncable), so a retired Property cannot hold an alert on for good.
2222

23-
import { and, desc, eq, gt, isNull, notInArray, sql } from 'drizzle-orm'
23+
import { and, desc, eq, gt, notInArray, sql } from 'drizzle-orm'
2424
import type { Database } from '#/shared/db'
2525
import { googleConnections } from '#/shared/db/schema/google-connection.schema'
2626
import { idempotencyReceipts } from '#/shared/db/schema/outbox.schema'
2727
import { properties } from '#/shared/db/schema/property.schema'
2828
import { reviews } from '#/shared/db/schema/review.schema'
2929
import { reviewGoogleReputationSnapshotFacts } from '#/shared/db/schema/review-reputation.schema'
30-
import {
31-
reviewFirstSightings,
32-
reviewSyncState,
33-
} from '#/shared/db/schema/review-sync.schema'
30+
import { reviewFirstSightings } from '#/shared/db/schema/review-sync.schema'
3431
import { propertyIsSyncable, propertySyncableSince } from '#/shared/db/syncable-property'
3532
import {
3633
PUSH_ALERT_WINDOW_MS,
@@ -226,8 +223,9 @@ async function readPushHealth(db: Database): Promise<SyncPushHealth['push']> {
226223
* head fact, Google's numbers from one read of its newest page, is no
227224
* reconciliation), or the day it qualified when
228225
* it has none, whichever is later (a restored Property is late from the day it
229-
* was restored, not from the fact it kept). A Property parked with a coded
230-
* failure is left to that failure's own alert.
226+
* was restored, not from the fact it kept). Failed Properties still count;
227+
* propertyIsSyncable excludes connections awaiting reauthorization, including
228+
* provider revocation, which has its own signal.
231229
*/
232230
async function readReconciliationAge(
233231
db: Database,
@@ -251,14 +249,7 @@ async function readReconciliationAge(
251249
})
252250
.from(properties)
253251
.innerJoin(googleConnections, eq(googleConnections.id, properties.googleConnectionId))
254-
.leftJoin(
255-
reviewSyncState,
256-
and(
257-
sql`${reviewSyncState.propertyId} = ${properties.id}::varchar`,
258-
eq(reviewSyncState.source, 'google'),
259-
),
260-
)
261-
.where(and(propertyIsSyncable, isNull(reviewSyncState.errorClass)))
252+
.where(propertyIsSyncable)
262253
return {
263254
overdueCount: row?.overdue ?? 0,
264255
oldestAgeMs: row?.oldestAgeMs == null ? null : Math.round(Number(row.oldestAgeMs)),

0 commit comments

Comments
 (0)