chore(pg): witness the swallowed mirror write, delete the membership reader, and give the dead rows a script (TASK-167) - #1948
Merged
Conversation
…reader, and give the dead rows a script (TASK-167) Three follow-ups from #1942, none of them a live defect. (1) The swallowed catch in `isPodMemberInMongo` is load-bearing and untested: removing the inner `try`/`catch` around `PGPod.addMember` left every suite green, yet that catch is what keeps a rejected cache write from reaching the outer catch, which answers 401 to a member Mongo lists. The rejection is ordinary — the FK on `pod_members.pod_id` fires when the pod has no PG row yet. The new arm drives it and asserts the member is admitted. (2) `PGPod.isMember` had zero production callers and three test arms. Deleted rather than relabelled: a method with that name on the PG model is what the next author reaches for, and the answer it gives authorises nothing. A source assertion with a positive control covers the absence, since no execution can show a method is gone. Its test arms went with it, and the four mirror mocks in the PG message suites were dropped — they mocked a reader that no longer exists. (3) `scripts/cleanup-ghost-pod-members.ts` removes the two dead row classes and names what it leaves. Dry run by default; `--apply` to write; the operator decides when. Built to the gate's three requirements: two numbers reconciled against the observed post-state, the survivor as the discriminating observation, and the orphan predicate separate from the ghost one with a read failure failing closed. Run only on the operator's word — nothing runs it.
The report already carried `reconciled: false`; `main()` printed it and returned success. So the more serious of the two conditions — the one that can only be discovered *after* rows are gone — reported success to anything reading the exit status, while `refused` (nothing written, re-run is safe) reported failure. The decision now lives in one exported function, so a new condition cannot be added to the log without being added to the status: refused -> 2 (nothing was written) reconciled === false -> 3 (something was written; inspect first) otherwise -> 0 Distinct codes so an operator reading a status can tell those apart without parsing logs; refusal wins if both are somehow true. `main(argv = process.argv)` so the status can be witnessed by a test without mutating the test runner's own argv. Arms (4): the diverged store exits 3; a clean store exits 0 (control, so 3 is not a blanket non-zero); a refused store exits 2; and the mapping table, including the both-true precedence. The first three call `main` for real with mongo mocked — the row's point is what the *process* reports, and the report had been right all along. Ledger against this head, 3 mutations each alone, no survivors: M8 process.exitCode hardcoded to 0 -> diverged arm + refused arm M9 divergence branch dropped -> diverged arm + mapping arm M10 refusal/divergence codes swapped -> all three arms The seven witnesses Vera already cleared were re-run against this head (23 -> 27 tests) and reproduced, with two new couplings worth noting: M3 (read failure classified as orphan) now also reddens the refused-exit arm, and M6 (reconciliation always true) also reddens the diverged-exit arm — the divergence is now witnessed in the report and in the status. One claim I wrote into a comment and then measured instead of trusting: a leaked `process.exitCode` does NOT make jest fail a green run (jest assigns its own status at teardown, exit 0 measured with the reset removed). The reset stays, to stop one arm reading a previous arm's status, and the comment now says what was measured rather than what was convenient.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cut from
a0a18011. Backend only, no version bump. The three carried follow-ups from #1942 (TASK-162), none of them a live defect — the code is correct; what was missing is a witness, a deletion, and a cleanup.(1) The swallowed cache write is now witnessed
isPodMemberInMongowrapsPGPod.addMemberin atry/catchthat is load-bearing and was pinned by nothing: removing it left 40/40 green. The rejection is ordinary, not exotic —pod_members.pod_idhas an FK ontopods(id), so a member of a pod with no PG row yet hits a constraint violation. Without the inner catch the rejection reaches the outer catch, whose job is to answer "not a member", and a legitimate member gets a 401 on read and on write.The new arm makes
addMemberreject with the real FK error and asserts: the attempt happened, the answer is not 401 and not 500, and the messages come back. Mutating the catch away reddens exactly this arm.Name correction from the gate, applied: the function is
isPodMemberInMongo(#1942 renamed it);isMemberWithFallbackno longer exists on main.(2)
PGPod.isMemberis deleted, not relabelledZero production callers on
a0a18011(the row's premise; the onlyisMember(hits inroutes/are a different function —utils/isPodMemberimported under that local name). It was a read of thepod_membersmirror, which has not decided anything since TASK-162, and a method with that name on the PG model is exactly what the next author reaches for when they want a membership answer.Deleted rather than renamed, for the reason the row gives: a renamed method with no caller and no test is one import away from being picked again. In its place is a comment saying what it was and what to name it if a mirror read is ever genuinely wanted.
What the deletion cost, disclosed. Three arms tested it and went with it (
PgPod.test.js,PgPodModel.more.test.js,PgPodModel.extra.test.js) — coverage of the deleted thing only. And four arms across the PG message suites mocked it to prove the controller doesn't consult it (PGPod.isMember.mockResolvedValue(true); // and would still say yes); those mocks and assertions are gone, because there is no longer a reader to mock — the controller suite automocks the PG model, so leaving them would have thrown. The property they witnessed (the PG row does not decide) survives as the arms' own assertions about the Mongo answer, and the structural claim is now pinned by source:An absence needs the source as the instrument (execution cannot show a method is gone), and the second assertion is the control that stops a typo'd pattern from passing as a deletion.
(3)
scripts/cleanup-ghost-pod-members.tsRemoves the two dead classes and names what it leaves: legitimate (pod in Mongo, user listed) → keep; ghost (pod in Mongo, user not listed) → delete; orphan (no such pod in Mongo) → delete. Idempotent. Dry run by default;
--applywrites; nothing runs it, and it runs on the operator's word.Built to the gate's three pre-specified requirements, each with its own witness:
examinedanddeleted, re-reads the row count after the sweep and reportsexamined - remaining === deletedreconciles the two numbers…(assertstrueon a consistent run andfalsewhen the store did not lose the row)(pod_id, user_id)pairs, never a bulkWHERE NOT INdeletes the ghost and the orphan and never the listed memberrefuses the whole run when a pod cannot be read, and deletes nothing even where it couldOne judgement the gate should see rather than infer: a malformed pod id is classified as an orphan, not as a read failure. Mongoose throws
CastErrorfor an id that cannot be an ObjectId, and that id cannot name a Mongo pod — so it belongs with "Mongo does not have it". Any other error is a failure to read and refuses the run. Getting that backwards makes the script permanently unable to run on a store with one legacy id, which would look like caution and be an outage of the tool.Ledger — seven mutations, each alone, no survivors
try/catcharound the mirror write removedadmits a listed member even when the mirror write is rejectedisMemberreader re-added to the PG modelexposes no membership reader: the mirror decides nothingrefuses the whole run when a pod cannot be read…a malformed pod id is an orphan, not a read failuredeletes the ghost and the orphan and never the listed memberandis idempotent…— both are deletion arms, and this is the mutation that collapses the differencetruereconciles the two numbers…a dry run classifies every row, names one example per class…Disclosure — my ledger's own instrument failed first, and it failed in the shape this PR is about. The harness that restores files between mutations was written as
open(dest, 'w').write(open(src).read()): Python opens the destination for writing (truncating the module under test) before it reads the backup, so a missing backup left the script file at zero bytes. The symptom was not an error from the harness — it was a mutation ofcontrollers/pgMessageController.tsreddening all six arms of the script's suite, which is what a suite looks like when its module is empty. I caught it because a controller mutation had no business touching those arms; the fix is read-then-write plus a non-empty assertion, and the table above is from the re-run against the final revision. Recorded because "the instrument did not run and printed a result" is now twice in one night (the other is in #1947).Runs and lint
origin/mainare 38, so +1 arm in the controller, +6 for the script, −3 for the deleted method, +1 structural.__tests__/unit— 424 suites / 3888 tests, green.__tests__/service+__tests__/services— 36 suites / 433 tests green (6 suites / 24 tests skipped, the real-DB tier)..ts: 0 errors, 0 warnings on any touched line; the new script is clean at 0/0 (threemax-lenwarnings from its own output lines were wrapped rather than shipped).fatalchecked..jstest files changed here carry the un-gated corpus'simport/no-unresolved+import/extensionserrors on.tsrequires, as does every file in that corpus.Witness for the not-from-a-fix half: nothing. These are hygiene items and the code they touch was already correct — the strongest claim available is that each mutation reddens its own arm and nothing else.
Gate: Vera.
Gate item: a failed reconciliation exits 0 (fixed at
e714c9e8)@vera found the report and the process disagreeing. The report carried
reconciled: false;main()printed it and returned success. So the more serious of the two conditions — the one that can only be discovered after rows are gone — reported success to anything reading the exit status, whilerefused(nothing written, re-run is safe) reported failure. A scripted caller reads the status; nobody greps the log line.The decision now lives in one exported function, so a new condition cannot be added to the log without being added to the status:
refusedreconciled === falseDistinct codes so the two can be told apart without parsing logs; refusal wins if both are somehow true.
main(argv = process.argv)so the status is witnessable without mutating the test runner's own argv.Arms (4): the diverged store exits 3; a clean store exits 0 (control, so 3 is not a blanket non-zero); a refused store exits 2; and the mapping table including the both-true precedence. The first three call
mainfor real with mongo mocked — the row's point is what the process reports, and the report had been right all along.Ledger against this head — 3 mutations, each alone, no survivors:
process.exitCodehardcoded to 0exitCodeForThe seven witnesses cleared at
46dab5c4were re-run against this head (23 → 27 tests) and all reproduced, with two new couplings: M3 (read failure classified as orphan) now also reddens the refused-exit arm, and M6 (reconciliation always true) now also reddens the diverged-exit arm — the divergence is witnessed in the report and in the status.One claim I wrote into a comment and then measured rather than trusting: I asserted that a leaked
process.exitCodewould make jest fail a green run. It does not — jest assigns its own status at teardown; measured exit 0 with the reset line removed. The reset stays (an arm must not read a previous arm's status) and the comment now says what was measured instead of what was convenient.