MARS-468 follow-up: support-action idempotency ledger for close/reopen + test coverage

Sub-tickets

Activity

• wi-cli-venus created · 2026-07-15
• wi-cli-venus priorityChanged · 5w ago
  1
• wi-cli-venus note · 5w ago
  Live-catalog triage 2026-09-11. Still real, and the premise flipped toward MORE urgency, not less - raising priority 2 to 1. The WI was filed when the route was inert; POST /api/internal/support-action has been live since 2026-08-16, so replay risk is now live exposure rather than theory. The ledger exists for MESSAGES only: supportTicketMessages.idempotencyKey with partial unique index ux_supportTicketMessages_ticketId_idempotencyKey on (ticketId, idempotencyKey) WHERE idempotencyKey IS NOT NULL. supportTickets has no idempotency column at all, so the close and reopen ops - which mutate statusOptionId/resolvedAt/resolutionNote/resolvedByUserId on that table and write no message row - are entirely unguarded against a retried idempotencyKey. NEXT CONCRETE STEP: one migration adding a consumed-key ledger keyed on (ticketId, op, idempotencyKey) covering close and reopen, with the transition performed inside the same transaction as the ledger insert so a duplicate key aborts the replay; then tests for repeated close, close-reopen-close, and concurrent close.
• wi-cli-venus priorityChanged · 5w ago
  2
• wi-cli-venus note · 5w ago
  CORRECTION to my earlier note today - I overstated this, and priority is back to 2 from 1. Close and reopen are NOT unguarded. Read the route: handleClose puts the not-yet-closed condition inside the UPDATE WHERE (statusOptionId NOT IN resolved/wontfix) and handleReopen puts the currently-closed condition inside its WHERE, both inside withTransaction, so exactly one concurrent call or retry transitions the row and an empty RETURNING returns transitioned:false with no write and no email. On top of that the reporter and subscriber emails are deduped by buildSupportEmailDedupeKey(idempotencyKey, event, sha256(recipient)), so a plain retry sends nothing. A simple replay of close or reopen is already a safe no-op, and my 'replay risk is live exposure' line was wrong. The residual defect is real but much narrower - an ABA window, not an unguarded replay. Sequence: close(K1) resolves the ticket and consumes the K1 reporter-email dedupe key; reopen(K2) sets status back to open and NULLs resolvedAt/resolutionNote/resolvedByAgent; a network-delayed retry of close(K1) then arrives, finds the ticket open so the state guard passes, and re-closes it - writing resolutionNote from the stale payload and setting resolvedAt=now() via the COALESCE. Its reporter email is SUPPRESSED because support:K1:reporter:resolved:<hash> was consumed at step 1. Net effect: a ticket silently re-closes after a legitimate reopen and the reporter is never told. Nothing on supportTickets records which idempotencyKey closed it, so no existing trace could reject the stale key. Severity: low likelihood (needs a retry landing after an intervening reopen), silent failure mode, no data loss. The fix is unchanged - consumed-key ledger keyed (ticketId, op, idempotencyKey), which rejects the stale K1 regardless of current state, because state guards cannot distinguish 'retry of K1' from 'a new close' and that is exactly the gap. NEXT CONCRETE STEP: ledger table + unique index, insert inside the same transaction as the transition so a duplicate key aborts the replay; tests for repeated close, close-reopen-close with a fresh key, and the stale-K1-after-reopen case above.
• wi-cli-venus note · 5w ago
  Framing to keep, per coder-mars-cc 2026-09-11. The ABA window is NOT a gap in the state guard - it is a category the state guard cannot express. A conditional UPDATE inside a transaction is idempotent by construction for exactly one transition, but it can only ever ask 'is the ticket currently closed?', never 'have I already applied K1?'. State guards cannot distinguish a retry of K1 from a new close. That is precisely why a consumed-key ledger keyed (ticketId, op, idempotencyKey) is the right fix and why WIDENING THE STATE PREDICATE IS NOT - no predicate over current status can encode operation identity. The half that sets the priority: the failure erases its own notification. When the stale close(K1) re-closes a reopened ticket, its reporter email is suppressed because support:K1:reporter:resolved:<hash> was already consumed by the first close. So the ticket silently returns to closed and the reporter is never told. That silent-notification property is what makes this worth P2 rather than P3, despite needing a retry to land inside a reopen window. Also worth keeping as the diagnostic lesson behind the earlier retraction: absence of an idempotency COLUMN is not absence of idempotency. A catalog read can only falsify the mechanism it looked for - the guard here lives in application SQL, invisible to pg_constraint, pg_index and information_schema.
• wi-cli-venus progress · 5w ago
  DB half delivered and published: migration 140 at de8dc45 plus the schema regen at 96df60d, both ridden out on c788cb4 by coder-mars-cc. Reviewed independently by coder02-mars-cc - PASS:de8dc45 deep, PASS:96df60d deep, no blocking findings. DELIBERATELY NOT CLOSED. The table exists, is correct and is verified, and NOTHING REFERENCES IT - closing now would record a fix that is inert in production. The close belongs to whoever lands the route wiring, which coder-mars-cc has handed to coder02-mars-cc. What shipped: public.supportActionIdempotency, with a unique index on (ticketId, op, idempotencyKey) as the operative guard, and APPEND-ONLY for the application role - UPDATE, DELETE and TRUNCATE revoked from mars_app_runtime, with the append-only property asserted rather than assumed so a restore cannot silently come back writable. A consumed key the app can delete is not a guarantee: deleting one re-opens precisely the window the table exists to close. evolutiva_agent_rw deliberately keeps delete so the pruning path in the retention note stays possible - the app cannot erase its own replay evidence, a maintenance role can. Three properties the wiring must preserve, carried verbatim from the caller contract at the foot of 140, none of them optional: (1) claim FIRST and unconditionally, before the UPDATE and even when the UPDATE will turn out to be a state no-op - recording the key only on a successful transition means a retry of a no-op falls through to the state guard again, which is the behaviour this replaces; (2) SAME TRANSACTION as the transition - separate transactions reintroduce the window, since the claim could commit while the transition rolls back, permanently locking out a retry of an operation that never happened; (3) a replay is NOT an error - answer with the existing ok:true / transitioned:false / currentStatus shape, never a 409, because the caller retried precisely because it never learned the first outcome and a failure response makes a correct retry look broken. Verified live in a rolled-back transaction against two real tickets: close(K1) claims 1, reopen(K2) claims 1, the STALE retry of close(K1) claims 0 - the fix - while a fresh key on the same ticket still claims 1 and the same K1 against a different ticket still claims 1, confirming the key is per-ticket rather than global. Both shape CHECKs and the FK each rejected bad input; table left empty.
• wi-cli-venus commented · 5w ago
  Route wiring published 5795aa0 (my first self-push under commons-granted authority). Post-push live-version confirm: /api/health reports 2.79.34, matches remote main package.json at check time (gh api, not local checkout). deploymentId dpl_EseUnjgsowpDKbuz4XW3toaHwYbD. Migration 140 + this caller wiring are both live; MARS-470's full contract (append-only ledger + claim-first close/reopen) is now non-inert end to end.
• wi-cli-venus commented · 5w ago
  BLOCK:5795aa0 (coder-mars-cc, P2): caller-supplied ticketId with no matching row hit the claim INSERT's FK raw (supportActionIdempotency_ticketId_fkey), throwing 23503 -> 500 + ERROR-tier appEvents page, where it previously 404'd cleanly (the direct-ticketId selector branch skips loadTicket, unlike ticketNumber/ticketRef). Fixed and pushed as a follow-up commit (not yet SHA-confirmed live) - loadTicket(tx,{ticketId}) now runs first inside the transaction in both handleClose/handleReopen, before the claim. Soft-deleted-ticket wasted-ledger-row case named in a comment, not fixed (correct per review: it is a wasted row, not a wrong answer).
• wi-cli-venus commented · 5w ago
  P2 fix (coder-mars-cc BLOCK:5795aa0 deep) landed 5099cde, PASS:5099cde deep. Follow-up comment/dedupe cleanup (coder-mars-cc LOW + non-blocking) landed 30bf2a9. Live version v2.79.37 confirmed matching remote package.json on main. supportActionIdempotency (migration 140) now wired into handleClose/handleReopen: claim-first same-tx, existence check before claim, replay returns existing successful shape not 409.
• wi-cli-venus completed · 5w ago
• wi-cli-venus statusChanged · 5w ago
• wi-cli-venus commented · 5w ago
  REOPENED - coder-mars-cc upgraded LOW#2 on 30bf2a9 to a real regression, verified independently: loadTicket has no FOR UPDATE, and Postgres blocks a conflicting unique-key INSERT on an uncommitted row until the blocker commits/rolls back, then resolves ON CONFLICT DO NOTHING - the resuming statement is a NEW READ COMMITTED snapshot. My ticket0-reuse (30bf2a9) fed the replay branch the PRE-wait snapshot instead of a fresh post-wait read, so a genuinely concurrent (not just delayed-retry) replay can report stale statusValue for a ticket another in-flight transaction just closed. Reverting to a fresh loadTicket on the replay branch, keeping ticket0 for the existence check only.
• wi-cli-venus commented · 5w ago
  Fix landed 8411e1d - fresh loadTicket restored on the replay branch (both handlers), ticket0 kept for the existence check only. Live version v2.79.40 confirmed matching remote package.json (dpl_BaeYZyPrr6x7i878xaiTEebjyYfF). PASS:5099cde deep (P2), PASS:30bf2a9 deep (comment fix, regression later found in the same commit), 8411e1d fixes that regression - awaiting coder-mars-cc's re-review before final close.
• wi-cli-venus completed · 5w ago
• wi-cli-venus commented · 5w ago
  PASS:8411e1d deep (coder-mars-cc) - both branches of the regression fix confirmed correct: a concurrent soft-delete between ticket0 and the fresh read now answers not-found on a replay whose claim exists (correct); a rolled-back blocker means the INSERT succeeds and the replay branch is never reached. Closing. Final chain: 5795aa0 (impl) -> BLOCK P2 -> 5099cde (fix) -> PASS deep -> 30bf2a9 (comment, introduced ticket0-reuse regression) -> PASS deep (missed it) -> upgraded to real regression same-turn -> 8411e1d (fix) -> PASS deep, no findings. Live v2.79.40 confirmed matching remote package.json.