Skip to content

The node comparison in InMemorySuspendedRunStore.claimSuspension is unpinned — deleting it alone leaves 61/61 green, and the pin file's comment claims the opposite (#14333 post-merge audit) #14956

Description

@os-sales

Filed by the domain:services execution seat from a post-merge tier audit of PR #14712 (#14333, merged 436841131 at 2026-09-03T01:16:55Z). Unassigned; domain:*, type and priority are triage's.

Lands in packages/services/service-automation — this lane. A follow-up PR against main; ⛔ #14712 is merged and must not be reopened or reused.

The measurement

In a detached worktree at feb213d42 (the merged head), deleting only this line from InMemorySuspendedRunStore.claimSuspension (around :147):

if (run.nodeId !== parkedAt.nodeId) return 'lost';

leaves the three-file population at 61 passed (61). Blob 0ad54b28…862753d5…, restore proven equal to HEAD.

For contrast, from the same bench: deleting both comparisons → 2 red; deleting the correlation comparison alone → 1 red; deleting the node comparison alone → 0 red.

The pin file says the opposite

concurrent-replica-resume-race.test.ts, the CONDITION block's lead comment, states the guarantee as "one per comparison, so a mutation that deletes only one of them still reddens."

That is false for the node half, and the reason is specific: both CONDITION cases re-park with a new correlation (req_lv1req_lv2, map:item_1map:item_2), so the correlation comparison rejects the stale claim first and masks the node comparison entirely. A comment describing a red that does not exist is worse than no comment — the next reader trusts it and will not re-derive it.

Why the gap is load-bearing, not theoretical

The maintainer's ruling is literally "delete only if still parked at node N", and SuspendedRunStore.claimSuspension's docblock explicitly allows a correlation-less parking:

a row persisted with no correlation has nothing to compare, and the node condition still holds

On that shape the node comparison is the only guard there is. Unpinned, a future refactor can delete it and every test stays green — on the exact code path whose whole purpose is stopping two replicas from resuming one run twice.

The pin already exists — the auditor wrote and verified it

A pausing executor that mints no correlation, with replica B's claim held until replica A has advanced and re-parked at the next node. It passes at HEAD and goes red under the node-only mutation (expected true to be false — B's stale claim granted, the action fires twice). Written in the PR's own harness idiom.

Suggested shape (for triage and whoever takes it, not a ruling):

  1. Add that pin.
  2. Rewrite the "one per comparison" sentence to the measured truth: both-deleted → 2 red; correlation-only → 1 red; node-only → 0 red until this pin exists.

⚠️ The ObjectStore side does not need it — dropping node_id from its where is caught by that suite's exact-shape assertions (measured: 6 red). This gap is specific to the in-memory store.

Also owed, and NOT VERIFIED as filed

The audit could not find the grouped follow-up this seat promised for the prior review's §5 notes 1 (the loser's hot-cache entry is not evicted on 'lost') and 5 (the non-count 'unsupported' branch re-deletes). A search found nothing. Whoever takes this card should check whether that finding exists and file it if not, rather than assuming either way.

Refs: PR #14712 / #14333 (merged; the audit's verdict comment carries the full bench) · the auditor's worktree convention scratchpad/review-14712-r2

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions