Roster CSV import is an unexercised bulk write path against trigger-archived tables - zero prod executions ever

Sub-tickets

Activity

• wi-cli-venus created · 7w ago
• wi-cli-venus progress · 7w ago
  Proof harness built and run; it found a REAL defect, and the fix shipped in v2.23.42 (PTD dpl_8su7Vr24Uc79JUYAwDxt8EfGSR4c). This WI expected to either close as exercised-and-sound or surface something - it surfaced something, and it is worse than the WI anticipated. DEFECT: applyImport silently discarded the ENTIRE batch on any one bad row WHILE REPORTING SUCCESS. Mechanism, all four steps needed for it to be silent: 1. The per-row catch collects the error and continues, but there was no SAVEPOINT anywhere in the codebase (grep: zero). 2. In PG a failed statement aborts the WHOLE transaction, so every row after the first failure returns 25P02 "current transaction is aborted". 3. COMMIT on an aborted transaction is executed AS a rollback and returns the command tag "ROLLBACK". 4. node-pg does NOT throw on that - it is a normal successful result - so the outer try/catch never fired. Net: return success:true with a nonzero created count, having written nothing. MEASURED, not reasoned. Local pg17 harness reproducing applyImport exact shape (BEGIN -> per-row set_config -> writes -> collect-and-continue -> COMMIT), 5 rows with row 3 a duplicate email. Two arms differing ONLY by the savepoint, which is what makes arm B evidence rather than a green run that never exercised anything: ARM A (production shape): reported success=true created=2, COMMITTED 0 ROWS. errors[] rows 4 and 5 read "current transaction is aborted". COMMIT command tag was "ROLLBACK". ARM B (per-row SAVEPOINT): committed rows 1,2,4,5, one real error, tag "COMMIT". FIX: per-row SAVEPOINT / RELEASE / ROLLBACK TO, taken BEFORE setArchiveContext so a rollback also unwinds that row set_config rather than leaving the previous row reason live. Plus a COMMIT-tag assertion returning success:false with errors[] preserved, so the admin still learns WHICH row to fix. The savepoints should make that guard unreachable; it asserts rather than trusts, because the failure it guards is a silent success report on a bulk write. REGRESSION TEST: src/lib/roster-import-savepoint.test.ts, 6 source-invariant assertions, each mutation-verified red (delete the opening SAVEPOINT / delete the ROLLBACK TO / neuter the commit guard). Worth recording: the FIRST draft was green with the opening SAVEPOINT deleted, because toContain("SAVEPOINT importRow") is satisfied by the ROLLBACK TO and RELEASE lines. The assertions now pin the full client.query(...) statement. A test that cannot fail is not coverage. SEVERITY READ: shipped-but-unexercised, not a live incident - zero prod executions ever (db-pluto-cc measurement in the WI description). But this is exactly the risk the WI was filed on: its first execution would have been on a real cohort with an admin watching, and the operator would have been told the import succeeded. The WI premise that zero prod rows is not evidence of correctness is now demonstrated rather than asserted. STILL NOT ESTABLISHED (the rest of the WI list, unchanged): that a multi-row import commits correctly against the REAL schema with its 23 archive triggers; that the per-row archive reason lands on every archived row (PLUTO-705/707 showed archiveReason NULLs through silently); that resolveImportRow conflict/error branches behave on real duplicate legajos/emails; that the transaction holds for a class-sized CSV without parking the pool client. My harness used a minimal 4-column table, NOT a restored Pluto schema - it was built to isolate the transaction semantics and that is all it proves. Keeping this WI open for the full-schema run. SPILLOVER: asked db-pluto-cc whether fn_bulkImportUsers and fn_approveEmailChangesBatch share the shape. Both return per-item errors[]/results[], i.e. both do collect-and-continue - safe ONLY if each iteration has its own plpgsql BEGIN...EXCEPTION block (implicit subtransaction). If either does not, it is the identical defect on a path that HAS run in prod. Awaiting their prosrc read; will file separately since DB-function fixes are their lane.
• wi-cli-venus progress · 7w ago
  Spillover CLOSED, no second instance. db-pluto-cc swept prosrc for every fn_* returning a per-item errors[]/results[] - only two exist, and both are SAFE by two DIFFERENT mechanisms, which is worth recording because only one of them is the fix I applied: - fn_approveEmailChangesBatch: per-item loop where each iteration is its own BEGIN ... EXCEPTION WHEN unique_violation / invalid_text_representation OR not_null_violation / OTHERS THEN <push error, no RAISE> END. A plpgsql BEGIN/EXCEPTION block creates an implicit subtransaction - that IS the per-row SAVEPOINT, spelled in plpgsql. - fn_bulkImportUsers: not an imperative loop at all. One set-based CTE chain that filters invalid rows out in a WHERE (roles_valid AND NOT email_exists) BEFORE the INSERT, then reports the excluded rows by diffing input against inserted. Nothing throws per row, so there is no aborted-transaction path to guard. So the defect was app-side only, and the app-side path was the one that had never been exercised in prod. The two that HAVE run are the two that were written safely - which is the same correlation the WI was filed on, seen from the other side. PLUTO-721 stays scoped to the fix already shipped in v2.23.42.
• wi-cli-venus progress · 7w ago
  FULL-SCHEMA RUN DONE, shipped v2.23.44 (PTD dpl_9UJo3HTf3qFr3uKGrvBDNo75i9wc). It found a SECOND defect, and it was one the FIRST fix made reachable. SETUP: pg_dump --schema-only from prod restored into local pg17 - 54 tables, 147 functions, 54 triggers, 1 harmless restore error. Seeded a minimal roster. The harness drives the REAL applyImport (auth guard, logger, email and domain-allowlist mocked; pool pointed at the restore), so every SQL statement, every archive trigger, fn_addUserRole and propagatePracticaAssignment all execute for real. ARM A - clean 8-row import: 8 users, 8 studentAssignments, 8 userRoles committed. success=true, errors=[]. The happy path works against the real schema. ARM B - genuine mid-batch DB failure: 4 rows written, ONE error naming rowNum 3, counters matching disk. This is the savepoint fix holding against real triggers rather than against a toy table. ARM C - archiveReason on a REPLACED assignment: the archive row carries the full bulk:rosterImport:batch:<uuid>:row:<n> with archivedBy set. The PLUTO-705/707 silent-NULL class does NOT reproduce on this path. That was an explicit WI unknown; it is now answered, negative. ARM D - THE NEW DEFECT. created and assigned are JS counters. ROLLBACK TO unwinds the row DB writes but cannot unwind a JS variable, so a row that failed AFTER its users INSERT still counted. Measured: a 4-row batch whose row 3 failed on the studentAssignments write reported created=4 with 3 users on disk. Not reachable before - without savepoints the whole transaction died rather than losing one row - so the fix that made rows fail independently is exactly what exposed the over-count. Now flagged per row and committed only after RELEASE. Both mutations verified red. TWO ARMS WERE GREEN WHILE EXERCISING NOTHING until I checked what they actually triggered. Recording both, because each is a false-negative generator for anyone testing this path again: 1. dotted-vs-undotted emails (dup@ vs d.u.p@) do NOT collide - canonicalizeEmail only strips dots for gmail, and these were @pensanta.com. Both inserted as separate users and the arm passed having triggered nothing. 2. a client-supplied matchedUserId pointing at a nonexistent user does NOT cause an FK violation - applyImport IGNORES it and re-resolves the match server-side through resolveImportRow. Worth knowing on its own: the import does not trust a client-chosen identity match. The shape that DOES fire: seed a persona row (ownerUserId set). loadImportLookupContext filters personas out with ownerUserId IS NULL, so the resolver cannot see it, falls through to INSERT, and collides with uq_users_emailcanonical - a row the app believes is new but the DB knows is not. Also worth having for anyone rebuilding the restore: fk_studentAssignments_comision_jtp is a COMPOSITE FK to comisionJtps(comisionId, jtpId), and comisiones itself is keyed (id, periodoId). Miss either seed row and EVERY assignment insert fails - which is how I first saw arm D, as 8 rows all failing at the same point. HARNESS COMMITTED at db/proofs/pluto-721-full-import.test.ts with the full restore+seed recipe in its header, plus vitest.proofs.mts to run it. Excluded from the default vitest config because it needs a database CI does not have; pnpm test still 733 green. REMAINING, and the only thing left on this WI: class-sized CSV under pool contention (DB_POOL_MAX=2, connectionTimeoutMillis=5s). Arms A-D used 4-8 rows, which does not hold the single client long enough to starve a sibling request. That is the last unexercised item from the original list.
• wi-cli-venus completed · 7w ago
  Closed on pm ruling 2026-08-22 10:24 ART: correctness is answered, which was the WI purpose; pool-contention arm skipped as a different test class with zero prod usage to date, revisit only if a real import gets scheduled. WHAT THIS WI SET OUT TO DO vs WHAT IT DID: it was filed to either close as exercised-and-sound or surface something. It surfaced TWO defects, and the second only existed because of the first fix. DEFECT 1 (v2.23.42) - the import silently discarded the ENTIRE batch on any one bad row while reporting success. Four steps, all needed for it to be silent: collect-and-continue with no SAVEPOINT anywhere in the codebase; one failed statement aborts the whole transaction so every later row returns 25P02; COMMIT on an aborted transaction executes AS a rollback and returns the command tag "ROLLBACK"; node-pg does not throw on that, so the outer catch never fired. Result: success:true with a nonzero created count, nothing written. Fixed with per-row SAVEPOINT/RELEASE/ROLLBACK TO plus a COMMIT-tag assertion. DEFECT 2 (v2.23.44) - created and assigned are JS counters, and ROLLBACK TO unwinds the row DB writes but cannot unwind a JS variable, so a row failing after its users INSERT still counted. Measured: 4-row batch, row 3 failing on the studentAssignments write, reported created=4 with 3 users on disk. UNREACHABLE BEFORE defect 1 was fixed, because the whole transaction used to die rather than lose one row. The general lesson, worth more than the fix: hardening an abort cascade CREATES a partial-success path, and partial success is where counter accounting starts lying. Any other collect-and-continue hardened this way will have the same second bug waiting behind it. VERDICT AND COVERAGE. Verified against a local pg17 restored from prod schema-only (54 tables, 147 functions, 54 triggers), driving the real applyImport with only the auth guard, logger, email and domain allowlist mocked - every SQL statement, every archive trigger, fn_addUserRole and propagatePracticaAssignment executed for real: - clean 8-row import commits 8 users / 8 assignments / 8 userRoles - a genuine mid-batch DB failure fails ALONE: 4 written, one error naming the right rowNum, counters matching disk - per-row archiveReason lands on the archive row of a replaced assignment with archivedBy set - the PLUTO-705/707 silent-NULL class does NOT reproduce on this path - counters no longer over-report a rolled-back row NOT exercised, stated rather than implied: a class-sized CSV under pool contention (DB_POOL_MAX=2, connectionTimeoutMillis=5s). Arms used 4-8 rows, which never holds the single pool client long enough to starve a sibling request. Also not exercised: realistic data volume, and the resolveImportRow conflict branches beyond the two shapes the arms hit. This is a correctness verdict, not a load verdict. SPILLOVER, closed negative: db-pluto-cc swept prosrc for every fn_* returning per-item errors[]/results[]. Only two exist and both are safe by different mechanisms - fn_approveEmailChangesBatch wraps each iteration in its own BEGIN/EXCEPTION block (an implicit subtransaction, the plpgsql spelling of the savepoint), fn_bulkImportUsers is set-based and filters invalid rows out before the INSERT so nothing throws per row. The two paths that HAVE run in prod are the two written safely; the one that never ran is the one that was broken - which is the correlation this WI was filed on, seen from the other side. ARTIFACT: db/proofs/pluto-721-full-import.test.ts, four arms, with the full restore+seed recipe in its header and vitest.proofs.mts to run it. Excluded from the default vitest config because it needs a database CI does not have. Two of its arms were GREEN WHILE EXERCISING NOTHING before I checked what they actually triggered - dotted-vs-undotted emails do not collide unless the domain is gmail, and a client-supplied matchedUserId is ignored because the import re-resolves the match server-side. Both dead ends are documented in the file so the next reader does not spend the same hour on them.