Skip to content

let the pin follow the lock instead of the review status #248

Description

@haribo

Why

A case at to-review that nobody has open stays frozen on an old edition,
and hides evidence that arrived after it.

Observed on dd9c192d95e0: the case was born before recordings were pushed and
sits at to-review, so it is pinned to a recording-less edition. The four
recordings are stored, in later editions. The reviewer needs the video to
review; the case waits for the review to show the video. The only way out today
is to accept the captures blind of the flow, which releases the case and makes
the videos appear — judging the images to earn the right to watch the flow.

The pin exists so a run landing mid-review does not change the bytes under a
reviewer (§7). But it keys on to-review, which says the ball is with the
reviewer
(ADR 0021) — not someone is looking right now. A case can sit there
for three weeks with nobody on it. We freeze bytes to protect a reviewer who
does not exist.

Occupancy now has its own answer: the lock, shipped by #247 (case_locks,
30-second heartbeat, two-minute expiry window). The pin should read that instead
of guessing from the status.

Decided, not to be reopened

The displayed edition becomes derived, not stored. Asked which edition a
case shows, the server answers from what is true at that moment:

  • a live lock on the case → the edition stamped when it was claimed;
  • otherwise → the project's latest edition.

Nothing to advance, nothing to catch up. A reviewer closes their laptop, the
heartbeat goes quiet, and the next read answers "the latest" with no write
having happened anywhere. This mirrors the lock's own rule: expiry is read,
never written (db/migrations/00021_a_case_is_held.sql).

Consequences, all of them intended:

  • case_locks gains edition_id, stamped at claim. It follows claimed_at:
    kept while the same holder's lock is still live (a heartbeat), re-stamped
    otherwise (a fresh claim, including after expiry).
  • cases.current_edition_id is removed, with AdvanceCurrentEdition
    (apps/server/db/queries/intake.sql:110) and ReleaseToLatestEdition
    (db/queries/aggregates.sql:237). They exist only to maintain the stored pin.
  • Intake stops skipping cases. Every case reads at the latest edition,
    because being to-review no longer freezes anything.
  • A delivery still advances the case at once (a delivery advances the case onto the edition that carries it #142). With no stored pin,
    deliver re-stamps the live lock onto the latest edition instead — same
    guarantee, one less mechanism. With no live lock there is nothing to do: the
    case already reads at the latest.

Rejected

  • An in-review cycle status. Forbidden by
    ADR 0005:
    occupancy is a separate axis and is never folded into the state field, or it
    destroys the state it replaces — a case at to-fix must still read to-fix
    while held.
  • Keeping the stored pin and adding the missing catch-ups — skip held cases
    at intake, release on lock release, and repair at read for locks that died in
    silence. Rejected: a state that repairs itself on read drifts again the first
    time somebody adds a read path, and this issue is a catch-up nobody wrote.
  • Showing a later edition's recording beside the pinned captures. Rejected:
    a hybrid view where the video shows a flow the captures do not show. The
    reviewer would judge an inconsistency the product invented.

Doc anchoring — propose, then wait for consent

Both files are under consent (CLAUDE.md). Propose the wording, get explicit
approval, then write, in the same diff as the code:

  • A new ADR (0024), completing ADR 0005: freezing the bytes follows
    occupancy, not the cycle state. Carries the two rejected alternatives above.
    Updates docs/adr/README.md in the same diff, and ADR 0005 gains the link.
  • docs/design/product.md §7, whose per-case paragraph today describes
    the pin as following to-review. It must describe the lock instead, and keep
    the a delivery advances the case onto the edition that carries it #142 delivery clause.

Build

Two central resolution points — the whole change lands there:

  • factsOf, apps/server/internal/adapters/postgres/deriver.go:25 — reads
    kase.CurrentEditionID, falls back to the latest.
  • resolveEdition, apps/server/internal/adapters/postgres/evidence.go:151
    same precedence, for the grid.

Both become one resolver: explicit edition asked for → live lock's stamped
edition → project's latest.

The stamping sites take that resolver instead of the removed column:
comment.go:136,147,240,513-521 and session.go:81-86,117-120,156-159.

Lock plumbing: db/queries/aggregates.sql:370-395
(ClaimCaseLock/ReadCaseLock/ReleaseCaseLock) and
internal/adapters/postgres/lock.go:39,73,84. ClaimCaseLock is also the
heartbeat, so the edition_id CASE rides alongside the existing claimed_at
one.

Migration: add case_locks.edition_id (nullable — a project with no edition can
be locked), drop cases.current_edition_id and its index
(00003_the_edition_a_case_is_judged_against.sql).

Existing tests this invalidates — approval required before touching them

Their premise is the status, and the premise is what changes:

  • TestAnIntakeDoesNotMoveTheBytesUnderAReviewer
    (postgres/edition_test.go:53) — must hold the lock to still mean anything.
  • TestACaseCatchesUpOnceItsReviewEnds (edition_test.go:86) — becomes "a case
    catches up as soon as nobody holds it".
  • TestAnOlderEditionDoesNotDrawAStepBornLater (intake_test.go:1043, draw only the steps that belong to the displayed edition #137) —
    built on a case pinned by its status; re-express with a lock.

Validation

  • A new test: a to-review case nobody holds reads at the latest edition, and
    shows a recording that landed after it was born. This is dd9c192d95e0.
  • A new test: a held case reads at the edition stamped at claim while a push
    lands, and reads the latest once the lock expires — no write in between.
  • TestADeliveryAdvancesTheCaseOntoItsEdition (reference_test.go:175) stays
    green, through the re-stamp.
  • grep -rn current_edition_id apps/server/ returns only the down migration.

Out of scope

  • Telling a holding reviewer that a newer edition is waiting. Real, and its own
    issue: it needs a UI decision, and this issue needs none.
  • Any change to what moved means, or to the recording verdict rules
    (ADR 0023).

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

    Labels

    type: featurenew capability or improvement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions