Work the design sweep's deferred backlog: audit-record loss, duplicate payout adds, heartbeat conflation - #163
Merged
Merged
Conversation
…ilure `logAudit(discord.role_changed)` sat inside the try, after both role loops. A member needing `+alumni -member` got the add applied; if the remove then threw, control jumped to the catch and both `counts.changed++` and the audit write were skipped. The role was added and `audit_log` said nothing happened — breaking the rule that every state change gets an audit write, and defeating PRODUCT.md's promise that an admin can answer "why is this person's role wrong?" from the log in under a minute. Both paths now accumulate what actually landed and write `role_changed` from the catch, with `partial` computed against what was expected rather than hardcoded — a complete strip whose audit write failed is not a partial one. The write sits above the permanence check deliberately: a transient error rethrows for pg-boss to retry, and the retry re-derives from current state, so it only ever audits the remainder. The strip path's success write moved INSIDE the try for the same reason. It previously sat below the whole try/catch, so a DB fault on that insert — the moment an audit write is most likely to fail — escaped with every removal landed and nothing recorded, and the retry then found nothing left to strip and reported "ok, removed 0". Both compensating writes are guarded and logged with correlatable ids. The sweep path's guard is load-bearing: an unguarded throw there escapes the row's catch and aborts the `for` loop, so every member not yet reached gets no role sync attempted at all this tick, and `errorSummary` carries that one DB fault instead of the accumulated per-member errors. It costs one row's audit trail, permanently — the next tick's diff comes back empty, restoring the state but never the record — to keep the rest of the sweep. Three earlier versions of that reasoning were wrong: that a retry recovers the lost rows, that the alternative was a false "ok" (the sweep can only return "partial" from there), and that the cost was recoverable next tick. Tests cover permanent mid-sweep, permanent mid-strip, transient mid-strip, and — via injected insert failures, including a persistent one that defeats the compensating retry — the swallow itself, asserting the loop still processes later members.
`logAuditIfChanged`'s docblock asked for an index on (action, target, id desc); only audit_log_at_idx existed. The lookup runs on the exceptional failure path only, but audit_log is append-only, so it degrades monotonically. Generated with `npm run db:generate`, never hand-written; no already-applied migration touched. The comments name what it actually serves — that lookup plus the two identity resolutions in services/audit.ts — and say plainly what it does NOT serve: /admin/audit's action filter is a LIKE prefix, and under en_US.utf8 a plain btree cannot answer `LIKE 'x%'` without text_pattern_ops. EXPLAIN puts it in Filter, not Index Cond. Both comments previously claimed the opposite, which would have sent the next person optimising that page at the wrong index.
…ver ran `workerHeartbeat` returned null for every error — the same value as "no worker has ever checked in". A permissions fault on pgboss.version therefore rendered "worker · no heartbeat recorded" for a healthy worker, on the page whose whole job is answering that question. Now a tagged union. 42P01 and an empty table still mean "never"; a read that failed is "error" and the page says so. An unparseable `maintained_on` is "error" too, not "never" — a non-null raw value means pg-boss wrote something, so the evidence exists and simply isn't parseable. Without that, an Invalid Date was tagged "ok" and left ageSec NaN to render. Postgres's `timestamptz` accepts 'infinity', so that path is reachable rather than hypothetical, and the e2e test uses it. The catch is kept: a DB fault on this auxiliary read must not take the page down. `queuedNotice` takes the error state as a required argument rather than inferring it from a null age, which both states produce. The drawer's re-run notice is why it matters — it returns that sentence as its entire output, with no worker line beside it, so an admin re-running a job during a read fault was told the deployment was young. The page lede had the same fault and now has its own branch instead of falling into "the worker is not running right now". `workerLine` is a switch with a `never` default, matching admin/accounts/actions.ts and worker/dispatcher.ts. The ternary it replaces keyed off a null age, which a future fourth variant would also satisfy — it would have compiled silently and fallen through to "no heartbeat recorded", reintroducing this exact regression word for word. actions.ts keeps its ternary, with a comment saying why that one is safe: it builds no sentence a new variant could fall into.
`defaultValue` compiles to the value attribute, which a browser ignores once the input's dirty value flag is set — which it is, the operator having typed in it. After a successful add the total and note were still on screen, so a second press of "Add flat pool" banked the same numbers twice, inflating a total real people get paid from. Same on add-participant, where each duplicate draws a full share. Controlled state now, following AppraiseForm, cleared in an effect only on `state.ok`. The rejection path needed no restore code: these forms settle through formAction, so nothing ever clears what was typed — the missing half was always the reset, never the restore. `FlatPoolEditState`'s rejection variant drops the three echo fields accordingly, since nothing reads them back; `StringFieldEditState` keeps `value`, which InlineEdit still uses. Both docblocks previously credited the echoed payload as the mechanism that preserved input. They now say staying mounted is necessary but not what preserves the values, and point at the controlled state that does. Covered by e2e rather than unit tests: the bug is browser DOM behaviour jsdom would not reproduce, and this project has no jsdom regardless.
`AdminAccountsDoneCode` was exported and derived from DONE_CODES, and
`isDoneCode` already guarded at runtime — but the function took a bare
string, so a typo'd literal in actions.ts typechecked and returned "".
One signature, `AdminAccountsDoneCode | undefined`, with `isDoneCode`
exported so page.tsx narrows the query string at the boundary — the one place
an unrecognised code can legitimately arrive.
An overload pair was tried first and reverted as inert: TypeScript resolves
overloads in declaration order, so a permissive second signature always
catches what the narrow one rejects, and `accountsConfirmation("teir", ...)`
still compiled clean. Verified this time by compiling a probe against the
real module — the typo now raises TS2345.
The test that passed an unrecognised code directly is split in two: the
boundary behaviour is tested through `isDoneCode` where it now lives, and the
defence-in-depth runtime fallback keeps a test with an explicit cast, since
that branch is unreachable by types on purpose.
SECOND-PASS.md exists so the next sweep recognises a known open item instead of rediscovering it as a new finding. Leaving it stale would defeat the one document that prevents that. Records what each fix decided, not just that it happened — the guard reasoning in discord-roles and why both paths ended up guarded for different reasons, that the add-forms' rejection path never needed restore code, and that the accountsConfirmation fix is a narrowed signature rather than the overloads an earlier draft of this file described. Adds the two findings this pass surfaced and chose not to fix, so the branch would stop growing: /admin/audit's action filter has no index that serves it (a LIKE prefix needs text_pattern_ops, a separate migration and a separate decision), and the heartbeat error variant's message has no reader.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR validates admin completion codes, models worker heartbeat outcomes explicitly, updates sync messaging, clears payout forms after successful submissions, adds audit-log indexing, and improves Discord role audit recovery across partial failures. ChangesWorker heartbeat status
Payout form state
Discord role audit handling
Admin completion-code validation
Audit-log lookup index
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AdminSyncPage
participant workerHeartbeat
participant PgBossDatabase
participant queuedNotice
AdminSyncPage->>workerHeartbeat: Request worker heartbeat
workerHeartbeat->>PgBossDatabase: Read maintained_on
PgBossDatabase-->>workerHeartbeat: Timestamp, null, or read error
workerHeartbeat-->>AdminSyncPage: Return WorkerHeartbeat status
AdminSyncPage->>queuedNotice: Pass freshness and heartbeat error state
queuedNotice-->>AdminSyncPage: Render worker status message
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drizzle/0009_useful_frightful_four.sql`:
- Line 1: Move creation of audit_log_action_target_id_idx out of Drizzle’s
transactional migration flow and execute it through a maintenance step or custom
runner using CREATE INDEX CONCURRENTLY, while preserving Drizzle migration
bookkeeping. If 0009_useful_frightful_four.sql has shipped, leave it unchanged
and add a follow-up migration or maintenance step that safely handles the
existing index.
In `@e2e/payouts.spec.ts`:
- Around line 302-346: Add a rejected flat-pool submission test near the
existing successful “a successful flat pool add clears the form” test, using the
flat-pool form flow to submit an invalid total value. Assert the `total_invalid`
error notice is shown and verify the total, required note, and optional contents
fields retain their entered values; keep the existing participant rejection
coverage unchanged.
In `@e2e/sync.spec.ts`:
- Around line 465-485: Wrap the heartbeat mutation, admin sync navigation,
assertion, and restoration in a finally block so setHeartbeat(ago(240 * MIN))
always executes, including when page.goto or toHaveText fails. Preserve the
existing infinity setup and expected notice assertion while ensuring the shared
pgboss.version state is restored.
In `@src/app/admin/sync/page.tsx`:
- Around line 190-192: Update the status message in the worker-status rendering
logic to avoid asserting that the worker is not running when status is "never".
State only that no heartbeat has been recorded yet and that queued work may be
delayed until the worker’s maintenance status is available, while preserving the
existing "error" message branch.
In `@src/app/payouts/actions.ts`:
- Around line 461-475: Update addFlatPoolAction to define and use a Zod schema
for caller-controlled inputs, including notes and totalValue, before calling
requireOperatorAccount or any other service. Replace the manual validation
branches with schema parsing and map validation failures to the existing
OperationErrorCode values, preserving the current total and note error behavior.
In `@src/jobs/discord-roles.ts`:
- Around line 362-391: Replace the lengthy debate in the auditErr catch within
the roles sync loop with a concise comment explaining the invariant and
trade-off: this row is already counted as failed, so guarding the audit write
sacrifices one unrecoverable audit row while allowing the remaining rows to
process; the next tick’s diffRoles will be empty because the roles already
changed. Preserve the intent that the failure is logged rather than silently
ignored.
- Around line 345-397: In src/jobs/discord-roles.ts lines 345-397, wrap the
failure-audit logAuditIfChanged call in its own try/catch and console.error any
audit-write failure so processing continues through rows. Apply the same guard
to logAuditIfChanged in src/jobs/discord-roles.ts lines 186-196, while
preserving the branch’s {status:"failed"} result and original err as the
reported cause.
In `@tests/admin-accounts-view.test.ts`:
- Around line 130-157: Add end-to-end coverage for AdminAccountsPage’s
query-parameter boundary, verifying a recognized ?done= value produces the
expected confirmation text and an unrecognized value renders no confirmation
text. Exercise the page through its public behavior so isDoneCode handling is
covered, and assert rendered text content rather than element counts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d2e0260-d2a4-4534-8657-a60bdc0beb17
⛔ Files ignored due to path filters (3)
docs/design-sweep/SECOND-PASS.mdis excluded by!docs/design-sweep/**drizzle/meta/0009_snapshot.jsonis excluded by!drizzle/meta/**drizzle/meta/_journal.jsonis excluded by!drizzle/meta/**
📒 Files selected for processing (20)
drizzle/0009_useful_frightful_four.sqle2e/error-boundary.spec.tse2e/payouts.spec.tse2e/sync.spec.tssrc/app/admin/accounts/page.tsxsrc/app/admin/accounts/view.tssrc/app/admin/sync/actions.tssrc/app/admin/sync/page.tsxsrc/app/admin/sync/view.tssrc/app/payouts/[id]/add-participant-form.tsxsrc/app/payouts/[id]/flat-pool-form.tsxsrc/app/payouts/actions.tssrc/db/schema.tssrc/jobs/discord-roles.tssrc/services/audit.tssrc/services/health.tstests/admin-accounts-view.test.tstests/discord-roles-job.test.tstests/health-service.test.tstests/sync-view.test.ts
Comment on lines
+465
to
+485
| await setHeartbeat(ago(2 * MIN)); // creates/tracks pgboss.version for cleanup | ||
| await db.execute(sql` | ||
| update pgboss.version set maintained_on = 'infinity' where version = 999999 | ||
| `); | ||
| await page.goto("/admin/sync"); | ||
| await expect(page.locator(".notice--bad .worker")).toHaveText( | ||
| "worker · heartbeat check failed — unknown whether the worker is running", | ||
| ); | ||
| // `resetDb` (the per-test `beforeEach` above) does not touch `pgboss`, so | ||
| // an unrestored 'infinity' would leak into every test that runs after this | ||
| // one in the file, not just the next — and several of them (e.g. "an | ||
| // overdue job...", right after this one) never call `setHeartbeat` | ||
| // themselves and rely on inheriting a STALE, not-fresh heartbeat from the | ||
| // immediately preceding test. Restore exactly that inherited state (what | ||
| // "the worker line reports liveness" above leaves behind: 240 minutes | ||
| // stale) rather than a fresh one, which would silently change | ||
| // `worker.fresh` for every test after this one in the file and was caught | ||
| // failing "an overdue job..." during review — a same-file example of why | ||
| // this file's own comments are this careful about what each test leaves | ||
| // behind. | ||
| await setHeartbeat(ago(240 * MIN)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the heartbeat in a finally block.
If page.goto() or toHaveText() fails, this test exits before Line 485. It then leaves maintained_on = 'infinity' in the shared pg-boss table. Later tests can fail with the wrong worker state.
Proposed fix
await asAdmin(context);
await setHeartbeat(ago(2 * MIN)); // creates/tracks pgboss.version for cleanup
- await db.execute(sql`
- update pgboss.version set maintained_on = 'infinity' where version = 999999
- `);
- await page.goto("/admin/sync");
- await expect(page.locator(".notice--bad .worker")).toHaveText(
- "worker · heartbeat check failed — unknown whether the worker is running",
- );
- await setHeartbeat(ago(240 * MIN));
+ try {
+ await db.execute(sql`
+ update pgboss.version set maintained_on = 'infinity' where version = 999999
+ `);
+ await page.goto("/admin/sync");
+ await expect(page.locator(".notice--bad .worker")).toHaveText(
+ "worker · heartbeat check failed — unknown whether the worker is running",
+ );
+ } finally {
+ await setHeartbeat(ago(240 * MIN));
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await setHeartbeat(ago(2 * MIN)); // creates/tracks pgboss.version for cleanup | |
| await db.execute(sql` | |
| update pgboss.version set maintained_on = 'infinity' where version = 999999 | |
| `); | |
| await page.goto("/admin/sync"); | |
| await expect(page.locator(".notice--bad .worker")).toHaveText( | |
| "worker · heartbeat check failed — unknown whether the worker is running", | |
| ); | |
| // `resetDb` (the per-test `beforeEach` above) does not touch `pgboss`, so | |
| // an unrestored 'infinity' would leak into every test that runs after this | |
| // one in the file, not just the next — and several of them (e.g. "an | |
| // overdue job...", right after this one) never call `setHeartbeat` | |
| // themselves and rely on inheriting a STALE, not-fresh heartbeat from the | |
| // immediately preceding test. Restore exactly that inherited state (what | |
| // "the worker line reports liveness" above leaves behind: 240 minutes | |
| // stale) rather than a fresh one, which would silently change | |
| // `worker.fresh` for every test after this one in the file and was caught | |
| // failing "an overdue job..." during review — a same-file example of why | |
| // this file's own comments are this careful about what each test leaves | |
| // behind. | |
| await setHeartbeat(ago(240 * MIN)); | |
| await setHeartbeat(ago(2 * MIN)); // creates/tracks pgboss.version for cleanup | |
| try { | |
| await db.execute(sql` | |
| update pgboss.version set maintained_on = 'infinity' where version = 999999 | |
| `); | |
| await page.goto("/admin/sync"); | |
| await expect(page.locator(".notice--bad .worker")).toHaveText( | |
| "worker · heartbeat check failed — unknown whether the worker is running", | |
| ); | |
| } finally { | |
| await setHeartbeat(ago(240 * MIN)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/sync.spec.ts` around lines 465 - 485, Wrap the heartbeat mutation, admin
sync navigation, assertion, and restoration in a finally block so
setHeartbeat(ago(240 * MIN)) always executes, including when page.goto or
toHaveText fails. Preserve the existing infinity setup and expected notice
assertion while ensuring the shared pgboss.version state is restored.
Comment on lines
+130
to
+157
| // `accountsConfirmation` itself is typed to `AdminAccountsDoneCode | | ||
| // undefined` now — a real caller can no longer pass an unrecognized string | ||
| // and have TypeScript let it through. The one boundary where an | ||
| // unrecognized `?done=` can legitimately arrive is `page.tsx`'s | ||
| // `params.done`, a raw query-string value, and that boundary narrows with | ||
| // `isDoneCode` before ever calling this function. So the "unrecognized | ||
| // code" behaviour is tested at that boundary directly, not through | ||
| // `accountsConfirmation`'s own signature. | ||
| it("isDoneCode rejects a done code this build doesn't recognize", () => { | ||
| // A hand-typed `?done=` (or one a future rollback no longer emits) must | ||
| // not silently pass through to become copy on the page. | ||
| expect(accountsConfirmation("delete_account", undefined, undefined)).toBe(""); | ||
| // not narrow into a code `page.tsx` would go on to pass through. | ||
| expect(isDoneCode("delete_account")).toBe(false); | ||
| expect(isDoneCode(undefined)).toBe(false); | ||
| }); | ||
|
|
||
| it("still degrades to no confirmation if an unrecognized code reaches it directly", () => { | ||
| // Defence in depth, not the load-bearing check any more — see this | ||
| // function's own docblock in view.ts. Exercised via a cast because the | ||
| // exported signature no longer admits an arbitrary string; the runtime | ||
| // guard inside still does, on purpose, in case a future caller adds a new | ||
| // code to `actions.ts` before `DONE_CODES` learns about it. | ||
| expect( | ||
| accountsConfirmation( | ||
| "delete_account" as AdminAccountsDoneCode, | ||
| undefined, | ||
| undefined, | ||
| ), | ||
| ).toBe(""); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an end-to-end assertion for the completion-code boundary.
These unit tests do not verify that AdminAccountsPage applies isDoneCode. Add an end-to-end test for a recognized ?done= value that asserts the confirmation text. Add an unrecognized-value assertion that confirms no confirmation text renders. Assert text content, not element count.
As per coding guidelines, “member- or admin-visible behavior should have an end-to-end assertion.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/admin-accounts-view.test.ts` around lines 130 - 157, Add end-to-end
coverage for AdminAccountsPage’s query-parameter boundary, verifying a
recognized ?done= value produces the expected confirmation text and an
unrecognized value renders no confirmation text. Exercise the page through its
public behavior so isDoneCode handling is covered, and assert rendered text
content rather than element counts.
Source: Coding guidelines
…-backlog # Conflicts: # drizzle/meta/0009_snapshot.json # drizzle/meta/_journal.json
…sating ones
Review caught that this PR guarded the new compensating `logAudit` calls and
left the `logAuditIfChanged` failure writes immediately below them unguarded
— so one DB fault still produced exactly the outcomes the new comments said
were prevented, arriving one statement later.
Sweep path: an unguarded throw there escapes the per-row catch and aborts the
`for` loop, abandoning every member not yet reached. That is the specific
thing the guard above it exists to prevent.
Strip path: the branch's own comment claims it returns `{status:"failed"}`
with the Discord error as the cause. Unguarded, a DB fault on that insert
propagates instead, and the Discord failure that actually happened is never
reported. The comment was only true once the write was guarded.
Also trims the guard comment down to the invariant and the trade-off. It had
grown into an argument with its own earlier revisions, which belongs in the
history rather than in the file.
…sent The sync lede said "the worker is not running right now" for every non-fresh state, including "never". That does not follow: pg-boss's maintenance loop writes on its own cadence, so a worker that started moments ago is processing work while `pgboss.version` is still empty, and the page would be asserting something it cannot support. Only a STALE heartbeat is positive evidence of a stopped worker — pg-boss wrote one and then stopped. "never" and "error" each now say what is actually known instead of borrowing that claim.
… failure Three gaps review found, all in this PR's own new coverage. The flat-pool form had a clears-on-success test and nothing for rejection — the more important half. The whole risk of converting these forms to controlled state was trading the duplicate-submit fix for a lost-input regression, so the rejection path is precisely what must not break. Now asserts the notice, that no pool was banked, and that all three fields still hold what was typed. `accountsConfirmation`'s narrowing moved the guard to the page boundary, but only unit tests covered it — nothing verified `AdminAccountsPage` actually applies `isDoneCode`. Two e2e tests now cover a recognised and an unrecognised `?done=`. Both scope to `ConfirmNotice`'s own focus wrapper: /admin/accounts mounts three unconditional `Notice`s, so a bare locator matches all three, and both assert text rather than element count — an empty state is also an element. The `'infinity'` heartbeat test restored its value as a trailing statement, so a failing assertion would exit first and leak it into every later test in the file, turning one real failure into a cascade that hides which test broke. Now `try/finally`.
`CREATE INDEX` takes a SHARE lock while it builds, and fly.toml runs migrations as a deploy-gating release command. Keeping the plain form is deliberate: CONCURRENTLY cannot run inside a transaction, Drizzle's migrator wraps every migration in one, and a failed concurrent build leaves an INVALID index needing manual cleanup in a release step nobody watches. Fine at current size — sub-second, on an index that serves only the exceptional failure path. Records the point at which that flips, and that the honest fix then is a custom migration runner rather than the COMMIT;-in-the- file hack. Raised by CodeRabbit on #163 and answered there.
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
…d the trigger (#166) Closes the open item in docs/design-sweep/SECOND-PASS.md section 4b. CodeRabbit raised it on #163, it was answered there, and the reviewer withdrew the finding — but the reasoning lived only in a PR thread and a "why deferred" table cell. Decision: keep drizzle's stock migrate(). Reading the migrator source makes the case stronger than the deferral recorded: - it wraps the ENTIRE pending batch in one transaction, not one per migration (pg-core/dialect.cjs: session.transaction sits outside the for-await loop) - __drizzle_migrations is read as a high-water mark (order by created_at desc limit 1), not a set of applied hashes So a custom runner's real risk is not the INVALID index, it is decoupling the DDL from the bookkeeping insert: a mid-batch failure commits statements the high-water mark still sits behind, the retry re-runs them and dies on "already exists", and the deploy wedges inside the gate. A failed CONCURRENTLY build is precisely the case that triggers it. Against that, the avoided cost is a sub-second SHARE lock. The COMMIT;-in-the-file hack is worse than inelegant: because the transaction spans the batch, it also commits every pending migration before it and runs every one after it untransacted. If the trigger ever fires, apply such an index out-of-band, watched, with a documented manual bookkeeping insert — failure mode stated in the doc. Also corrects two prose claims found while writing this up: sync_run is not purged (so it is unbounded too, just slower), and 0002/0003 build indexes against tables created in 0000, so those were not necessarily empty either. No change to how migrations are applied. src/db/migrate.ts gains a comment only.
This was referenced Aug 6, 2026
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
1 participant
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.
Works the open items carried in
docs/design-sweep/SECOND-PASS.mdfrom the Aug-5 sweep (#128), rather than re-running the sweep — that review was still current, so re-reviewing would have re-derived it and risked re-flagging entries indocs/settled-design-decisions.md. Each item was verified as still-live against current code first; 25 commits had landed since the doc was written.What this fixes
Audit log lost Discord role changes that actually happened (
198ae03)logAudit(discord.role_changed)sat inside the try, after both role loops. A member needing+alumni −membergot the add applied; if the remove then threw, the audit write andcounts.changed++were both skipped. The role was added andaudit_logsaid nothing happened — breaking the rule that every state change gets an audit write, and defeating PRODUCT.md's promise that an admin can answer "why is this person's role wrong?" from the log in under a minute.Both paths now record what actually landed, with
partialcomputed rather than hardcoded. The strip path's success write moved inside the try — it previously sat below the whole try/catch, so a DB fault on that insert escaped with every removal applied and nothing recorded, and the retry then found nothing left to strip and reportedok, removed 0.Duplicate loot pool / participant on a double press (
8f48c84)defaultValuesets the value attribute, which a browser ignores once the input's dirty flag is set. After a successful add the numbers were still on screen, so a second press banked them twice — inflating a total real people get paid from.A failed heartbeat check read as "worker never ran" (
e4f10db)workerHeartbeatreturnednullfor every error, the same value as "no worker has ever checked in", so a permissions fault rendered "no heartbeat recorded" for a healthy worker — on the page whose whole job is answering that.Supporting —
audit_logindex (22f7392),accountsConfirmationnarrowing (4ed2cd3), doc updates (7c3b062).Reviewer focus
src/jobs/discord-roles.tsis the substance. Both compensating audit writes are guarded and logged with correlatable ids, for different reasons stated at each site. The sweep path's guard is load-bearing: an unguarded throw escapes the row's catch and aborts theforloop, so members not yet reached get no sync attempted at all. It costs one row's audit trail — permanently, since the next tick's diff comes back empty — to keep the rest of the sweep.drizzle/0009_useful_frightful_four.sqlis generated, never hand-written; no applied migration touched. It runs as a Fly release command and briefly blocks writes toaudit_logwhile building. Deliberately notCONCURRENTLY, which can't run inside the migrator's transaction.Verification
Worth knowing
Most of what's here was found reviewing the fixes, not the original code — a type narrowing that narrowed nothing (an overload pair TypeScript resolves straight past), an index whose stated beneficiary it didn't serve (
EXPLAINshowsFilter, notIndex Cond— aLIKEprefix needstext_pattern_ops), two tests asserting against a cleared mock and passing vacuously, and nine wrong comment claims.That last number is the generalisable bit. This codebase deliberately encodes its reasoning in prose — it's why
settled-design-decisions.mdexists — and prose is the one artifact with no CI behind it. Every wrong claim here passed a human read and none was catchable by typecheck, lint, or tests.Left open, recorded in
SECOND-PASS.mdtext_pattern_opsindex for/admin/audit's action filter — separate migration, separate decision"error"variant'smessagefield has no readerSummary by CodeRabbit
New Features
Bug Fixes