DM-56095: Stop edition_reconcile publishing editions deleted mid-tick

docs/edition-reconcile.md

@@ -132,10 +132,10 @@ that tombstones a project's editions shipped without a backfill, so a
 project deleted before it still owns editions reading NULL. Left in the
 live legs those editions look like the plainest drift there is and earn
 a republish nothing can satisfy: `publish_edition` resolves the project
-by slug through a lookup that filters tombstones, so the job raises
-before it can mark the pair failed, the reaper fails the row an hour
-later, and the next tick re-drives the same pair — Sentry noise and a
-burnt action-cap slot, every tick, forever. Such an edition is therefore
+by slug through a lookup that filters tombstones, so the job retires
+as a `deleted_skipped` skip with the pair still unpublished, and the
+next tick re-drives the same pair — a wasted job and a burnt action-cap
+slot, every tick, forever. Such an edition is therefore
 unpublished if the edge still serves its key, and counted `tombstoned`
 otherwise.
 
@@ -167,36 +167,52 @@ drifting faster than one tick can repair.
 The decision table above describes an instant that has passed by the
 time an action is applied. The plan's read transaction commits before
 the CDN read-back, and up to a whole cap's worth of other actions can
-run before any one republish reaches the queue. Two things can change
+run before any one republish reaches the queue. Three things can change
 in that window, so each republish is re-tested as the first statement
 of the transaction that enqueues it — inside
 `enqueue_publish_for_edition`'s Phase A, before it writes anything:
 
 | Re-check | Dropped when | Reported as |
 | --- | --- | --- |
-| `editions.current_build_id`, read `FOR UPDATE` | the edition no longer points at the build the plan chose | `superseded_skipped` |
+| `editions.date_deleted` and its project's, read `FOR UPDATE` of the edition | the edition or its project has been soft-deleted | `deleted_skipped` |
+| `editions.current_build_id`, from the same locked read | the edition no longer points at the build the plan chose | `superseded_skipped` |
 | a live `publish_edition` job for the pair (row 6's test, asked of one pair) | another driver enqueued the pair since the snapshot | `in_flight_skipped` |
 
 A dropped action writes nothing at all: the edition is not marked
 `pending`, no history row is touched, and no `queue_jobs` row is
 created. It is logged at **info** with the edition and both build ids,
 and the tick carries on with the org's remaining actions.
 
-The lock is what makes the first check hold. Every repoint on
-`editions` — tracking, an API rollback, keeper-sync — goes through an
-`UPDATE` of that row, so it waits on the lock rather than slipping
-between the check and the queue insert, and the enqueue commits
-together with the current build it names. Without it the reconciler
+The first check covers a delete that lands mid-tick, most often the
+`ref_deleted` webhook for a branch removed as its pull request merges.
+The delete removes the edition's key as well as stamping the row, so
+the read-back that follows the plan's transaction finds no pointer for
+an edition the plan still holds as live and published — row 11's
+`pointer_missing`. Without the check the tick would mark the tombstone
+`pending` and enqueue a publish for an edition `publish_edition` can no
+longer resolve. The drop is counted `deleted_skipped` rather than
+`tombstoned`, so it is not lost among the org's settled tombstones; the
+next tick's plan reads the edition as an ordinary tombstone, and removes
+its key if the delete left one behind.
+
+The lock is what makes the first two checks hold. Every soft-delete and
+every repoint on `editions` — tracking, an API rollback, keeper-sync —
+goes through an `UPDATE` of that row, so it waits on the lock rather
+than slipping between the check and the queue insert, and the enqueue
+commits together with the state it rests on. Without it the reconciler
 could enqueue a publish of superseded content, and the `EDITION_UPDATE`
 advisory lock would not save it: that lock serializes publish jobs by
 pickup order, not enqueue order, so the stale job can win and leave the
 edge on the old build until the next tick notices.
 
-The second check is a point read, not a mutex. `queue_jobs` has no
+The last check is a point read, not a mutex. `queue_jobs` has no
 partial unique index for `publish_edition` and this loop deliberately
 does not add one — publish jobs for a pair are legitimately enqueued by
 several drivers, and the reconciler only needs to not be one more.
 
+`deleted_skipped` above zero is normal wherever branches are deleted
+as their pull requests merge, and is not a failure either.
+
 `superseded_skipped` above zero is normal on a busy org and is not a
 failure: it means editions moved while the tick walked them. A steady
 trickle argues for a lower `edition_reconcile_max_actions_per_job`, so
@@ -265,6 +281,7 @@ each carries the tally as `progress`:
   "unpublish_failed": 0,
   "in_flight_skipped": 0,
   "superseded_skipped": 0,
+  "deleted_skipped": 0,
   "grace_skipped": 1,
   "failed_left_alone": 2,
   "retired_build_skipped": 0,
@@ -308,7 +325,8 @@ event — including the ticks that found nothing, because a reconciler
 that goes quiet when it finds nothing is indistinguishable on a
 dashboard from one that stopped running. Its fields are
 `editions_scanned`, `pointers_read`, `republished`, `unpublished`,
-`in_flight_skipped`, `superseded_skipped`, `failed_left_alone`,
+`in_flight_skipped`, `superseded_skipped`, `deleted_skipped`,
+`failed_left_alone`,
 `unexpected_pointers`, `capped`, `cdn_checked`, and `elapsed`.
 
 `project` is always null: one tick spans every project in the org. The
@@ -332,8 +350,9 @@ much of an environment's publish traffic is the system healing itself.
   publish"` with its `reason`, or `"Removed a stranded edition
   pointer"`), tagged `phase="reconcile"`.
 - So does each action the apply-time re-check dropped —
+  `"Skipped a republish for a soft-deleted edition"`,
   `"Skipped a superseded edition republish"` or
-  `"Skipped an in-flight edition republish"` — both carrying
+  `"Skipped an in-flight edition republish"` — all carrying
   `edition_id`, `planned_build_id`, and the `current_build_id` read
   under the lock.
 

docs/metrics.md

@@ -715,6 +715,7 @@ explains each counter against the loop's decision table.
 | `unpublished` | integer | field | Stranded CDN keys the tick deleted. |
 | `in_flight_skipped` | integer | field | Pairs a live `publish_edition` job still held. |
 | `superseded_skipped` | integer | field | Planned republishes the edition had moved off before the enqueue. |
+| `deleted_skipped` | integer | field | Planned republishes whose edition was soft-deleted before the enqueue. |
 | `failed_left_alone` | integer | field | Pairs reading `failed`; reported, never re-driven. |
 | `unexpected_pointers` | integer | field | Keys for editions with no build to publish; reported, not acted on. |
 | `capped` | integer | field | Actions the per-job cap left for the next tick. |

server-changelog.d/20261110_210000_DM_56095_reconcile_tombstone_race.md

@@ -0,0 +1,8 @@
+### New features
+
+- The `edition_reconcile_completed` metrics event and the `edition_reconcile` job's `progress` gain a `deleted_skipped` count: republishes the tick planned and then dropped because the edition, or its project, was soft-deleted before the enqueue.
+
+### Bug fixes
+
+- `edition_reconcile` no longer enqueues a publish for an edition that was soft-deleted while the tick was running. A branch deleted as its pull request merged could be read as live by the tick's plan and then lose its CDN key to the delete webhook, which looked like a missing pointer; the tick re-drove the publish and `publish_edition` raised `NotFoundError` into Sentry. The apply-time re-check now reads the edition's `date_deleted`, and its project's, under the same row lock as `current_build_id` and drops the republish with nothing written, counting it as `deleted_skipped`. See `docs/edition-reconcile.md`.
+- `publish_edition` now completes with `deleted_skipped` in its `progress` when the project or edition its payload names was deleted between the enqueue and the pickup, or when the edition slug now belongs to a different edition. Before, it raised `NotFoundError` and left its `queue_jobs` row `in_progress` until `publish_edition_reaper` failed it four hours later; now the row closes at once and a keeper-sync run waiting on the job still finalises. The job reads its edition under a row lock, so a delete still uncommitted at pickup is waited for and seen rather than published over.

src/docverse_server/domain/edition_reconcile.py

@@ -19,6 +19,7 @@
     "EditionReconcilePlan",
     "EditionRepublish",
     "EditionUnpublish",
+    "LockedRepublishTarget",
     "ReconcileEdition",
     "ReconcileReason",
     "plan_edition_reconcile",
@@ -74,7 +75,7 @@ class ReconcileEdition:
     a project deleted before it still owns editions reading NULL, and
     those editions are unpublishable: ``publish_edition`` resolves the
     project by slug through a lookup that filters tombstones, so a
-    republish raises before it can record a failure and the pair is
+    republish retires as a skip with the pair still unpublished and is
     re-driven on every subsequent tick. Carrying the stamp lets the
     planner treat the whole subtree as tombstoned instead.
     """
@@ -205,6 +206,23 @@ class EditionRepublish:
     reason: ReconcileReason
 
 
+@dataclass(frozen=True, slots=True)
+class LockedRepublishTarget:
+    """What a planned republish rests on, re-read under the edition's lock.
+
+    The answer to the apply-time re-check: the plan chose its pair in a
+    transaction that has closed, so the enqueue asks again, under a row
+    lock, whether the edition is still there and still points where the
+    plan said.
+    """
+
+    current_build_id: int | None
+    """The build the edition points at now, if any."""
+
+    deleted: bool
+    """Whether the edition or its project has been soft-deleted."""
+
+
 @dataclass(frozen=True, slots=True)
 class EditionUnpublish:
     """One stranded CDN key the loop will delete.
@@ -413,8 +431,9 @@ def _classify_edition(
     :attr:`~ReconcileEdition.tombstoned`, which covers the edition's own
     stamp *and* its project's: a live-looking edition under a deleted
     project cannot be republished (the publish job resolves the project
-    through a tombstone-filtering lookup and raises), so routing it
-    anywhere but here would re-drive an unsatisfiable job every tick.
+    through a tombstone-filtering lookup and retires as a skip), so
+    routing it anywhere but here would re-drive an unsatisfiable job
+    every tick.
     """
     if edition.tombstoned:
         if pointer is not None:

src/docverse_server/metrics/payloads.py

@@ -268,6 +268,14 @@ class EditionReconcileCompletedEvent(DocverseEventBase):
     them — a reason to lower the per-job cap, not a failure.
     """
 
+    deleted_skipped: int
+    """Planned republishes whose edition was deleted before the enqueue.
+
+    Soft-deletes that landed after the plan was made, most often the
+    delete webhook for a branch removed as its pull request merged. Not
+    a failure: the tick stood down instead of publishing a tombstone.
+    """
+
     failed_left_alone: int
     """Pairs reading ``failed``; reported for operators, never re-driven."""
 

src/docverse_server/services/edition_reconcile.py

@@ -18,8 +18,8 @@
 transaction, because the plan describes an instant that has passed:
 the read transaction closed before the CDN read-back, and up to a
 whole cap's worth of other actions may have run since. An edition
-repointed in that window, or a pair another driver has meanwhile
-enqueued, is dropped rather than re-driven — see
+soft-deleted or repointed in that window, or a pair another driver has
+meanwhile enqueued, is dropped rather than re-driven — see
 :meth:`EditionReconcileService._recheck_republish`.
 
 An unpublish has no such path to defer to; nothing enqueues a key
@@ -113,17 +113,23 @@ class _ApplySkip(StrEnum):
     named to match it. ``in_flight`` feeds the very counter the
     planner's ``in_flight_skipped`` bucket feeds, because a pair skipped
     for a live publish is the same finding whichever of the two reads
-    noticed it. ``superseded`` has no plan-time sibling: the planner
-    cannot observe a repoint that happens after its own transaction
-    closed.
+    noticed it. ``deleted`` and ``superseded`` have no plan-time
+    sibling: the planner cannot observe a soft-delete or a repoint that
+    happens after its own transaction closed. ``deleted`` is kept out of
+    the planner's ``tombstoned`` bucket on purpose — that one is a
+    standing count of an org's settled tombstones, and folding a race
+    into it would hide how often the race fires.
     """
 
+    deleted = "deleted"
     superseded = "superseded"
     in_flight = "in_flight"
 
     @property
     def message(self) -> str:
         """The log line this skip is reported under."""
+        if self is _ApplySkip.deleted:
+            return "Skipped a republish for a soft-deleted edition"
         if self is _ApplySkip.superseded:
             return "Skipped a superseded edition republish"
         return "Skipped an in-flight edition republish"
@@ -146,7 +152,7 @@ def __init__(
     ) -> None:
         super().__init__(f"Republish dropped at apply time: {skip.value}")
         self.skip = skip
-        """Which of the two apply-time races dropped the action."""
+        """Which of the apply-time races dropped the action."""
 
         self.current_build_id = current_build_id
         """The build the edition actually pointed at under the lock."""
@@ -208,6 +214,20 @@ class EditionReconcileOutcome:
     per-job cap wants lowering rather than that anything is broken.
     """
 
+    deleted_skipped: int = 0
+    """Planned republishes whose edition was soft-deleted before the enqueue.
+
+    The other apply-time bucket with no planner counterpart (issue
+    #886): the edition, or its project, was deleted after the plan's
+    transaction read it as live — most often by the delete webhook for a
+    branch removed as its pull request merged. Kept apart from
+    :attr:`tombstoned`, which counts an org's settled tombstones and
+    would bury a handful of these in a stock of thousands. Nothing is
+    unpublished for such an edition this tick; if the delete left its
+    key behind, the next tick's plan reads it as a tombstone and removes
+    it.
+    """
+
     grace_skipped: int = 0
     """Pairs too young to tell apart from an enqueue in progress."""
 
@@ -285,6 +305,7 @@ def as_progress(self) -> dict[str, Any]:
             "unpublish_failed": self.unpublish_failed,
             "in_flight_skipped": self.in_flight_skipped,
             "superseded_skipped": self.superseded_skipped,
+            "deleted_skipped": self.deleted_skipped,
             "grace_skipped": self.grace_skipped,
             "failed_left_alone": self.failed_left_alone,
             "retired_build_skipped": self.retired_build_skipped,
@@ -503,7 +524,9 @@ async def _recheck() -> None:
                 precheck=_recheck,
             )
         except _StalePlanError as stale:
-            if stale.skip is _ApplySkip.superseded:
+            if stale.skip is _ApplySkip.deleted:
+                outcome.deleted_skipped += 1
+            elif stale.skip is _ApplySkip.superseded:
                 outcome.superseded_skipped += 1
             else:
                 outcome.in_flight_skipped += 1
@@ -551,11 +574,21 @@ async def _recheck_republish(
         Runs as the first statement of
         :func:`~docverse_server.services.publish_enqueue.enqueue_publish_for_edition`'s
         Phase A, and raises :exc:`_StalePlanError` to abort it with
-        nothing written. Both conditions it tests are races the plan
+        nothing written. The conditions it tests are all races the plan
         cannot close on its own, because the plan's transaction commits
         before the CDN read-back and before the first of up to ``limit``
         enqueues:
 
+        * The edition, or its project, may have been **soft-deleted** —
+          most often by the delete webhook for a branch removed as its
+          pull request merged. The delete also removes the CDN key, and
+          the read-back that follows the plan's transaction then finds
+          no pointer for an edition the snapshot still calls live, which
+          is precisely the ``pointer_missing`` drift. Enqueuing would
+          mark a tombstone ``pending`` and hand ``publish_edition`` a
+          job for an edition it can no longer resolve (issue #886). A
+          soft-delete is an ``UPDATE`` of the locked row, so one
+          arriving mid-transaction waits like a repoint does.
         * The edition may have been **repointed** — by tracking, by an
           API rollback, by keeper-sync — since the pair was chosen.
           Enqueuing the planned build then publishes superseded content,
@@ -572,13 +605,24 @@ async def _recheck_republish(
           would get two jobs, two KV writes, and two
           ``edition_published`` events.
 
-        Order matters: an edition that has moved on is superseded
-        whatever is in flight for the build it has left, and that is the
-        more specific thing to tell an operator.
+        Order matters: a deleted edition has nothing to publish whatever
+        it points at, an edition that has moved on is superseded
+        whatever is in flight for the build it has left, and each is the
+        more specific thing to tell an operator. An ``edition_id`` that
+        names no row at all is reported as deleted too: there is nothing
+        to publish for it either.
         """
-        current_build_id = await self._edition_store.lock_current_build_id(
+        target = await self._edition_store.lock_republish_target(
             edition_id=action.edition_id
         )
+        if target is None or target.deleted:
+            raise _StalePlanError(
+                _ApplySkip.deleted,
+                current_build_id=(
+                    target.current_build_id if target is not None else None
+                ),
+            )
+        current_build_id = target.current_build_id
         if current_build_id != action.build_id:
             raise _StalePlanError(
                 _ApplySkip.superseded, current_build_id=current_build_id