Add anonymous multi-user vote command - #191
Conversation
Allow Admin/CM/Community Team to start timed anonymous polls so staff can approve or reject nominees without revealing voter identity or live tallies until close. Co-authored-by: Cursor <cursoragent@cursor.com>
|
can't code so had to use cursor. This is to help FalconSpy with our CC program via discord to keep moderation team votes anonymous. PLEASE ASSESS AND VERIFY CODE BEFORE PUSHING TO MAIN. |
|
@MetaspIoit I've reviewed the PR and got some comments. Do you prefer me to post them, so you can try to fix them, or do you prefer me to take over? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 66.54% 69.79% +3.25%
==========================================
Files 54 56 +2
Lines 3177 3536 +359
==========================================
+ Hits 2114 2468 +354
- Misses 1063 1068 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Concurrency and correctness fixes on top of the initial implementation, plus tests for the command and the view. Voting: - Read the chosen nominee from each interaction's own payload. py-cord shares the Select item across all voters and overwrites its state per interaction while callbacks run in later tasks, so two voters could cross nominees. - Move Approve/Reject onto a private per-voter ballot, replacing a module-level dict of pending selections that was lost on restart. - Re-check VOTE_CASTERS when a ballot is cast, not only when it is issued, so a voter whose role is revoked mid-poll cannot still cast. - Give the ballot an on_timeout. A finished view is evicted from the ViewStore, so a later click received no response at all and Discord showed 'This interaction failed' on buttons that still looked live. - Defer before database work in every callback; AsyncSessionLocal uses NullPool and Discord's initial-response deadline is 3 seconds. Persistence: - Write ballots as a single upsert so a double-click cannot race uq_anonymous_vote_ballot_session_candidate_voter. - Claim the close with a conditional UPDATE. The previous read-check-write straddled two awaits, and on_ready reschedules a close on every reconnect, so a long-running vote could publish its results more than once. - Store closes_at as BIGINT epoch seconds, matching Ban.unban_time, instead of a TIMESTAMP that caps at 2038 and round-trips through session timezones. - Delete the session row when the poll cannot be posted, rather than leaving it orphaned with no message id and no scheduled close. Input handling: - Bound topic to 200 characters. build_results_embed prefixes 'Results: ', so a longer topic exceeded the 256-character embed title limit and failed at close time, losing the tallies. - Cap vote duration at 30 days and require nominee IDs to be snowflake-shaped. Adds 72 tests covering the command and the view, including the approve/reject tally, the activity-box truncation boundary, and the concurrency guards.
|
Hi @MetaspIoit — thanks for this, the feature and the data model are a good shape and I wanted it to land. I've pushed a commit to your branch ( Concurrency
Discord API constraints
Storage
TestsAdded 72 tests for the command and the view. Worth flagging one thing I got wrong first time: the tally logic ( Two things that were your calls, not mineI'd rather ask than quietly redesign, and I'm happy to revert either. 1. Approve/Reject moved from public buttons to a private per-voter ballot. The crossing-nominees bug only strictly required reading from 2. Vote duration is capped at 30 days. Nothing forced this — One note on the test planThe checklist in the description now describes the old flow. If you're working through it: step 4 gives you a private ballot rather than buttons on the poll message, and there are two new paths worth a look — Known limitation, unchanged from your designWith a small pool of eligible voters, the live per-nominee activity boxes leak participation — you can tell how many people voted on each nominee, though not which way. The approve/reject split stays hidden until close. I left this as you built it; say the word if you'd prefer an aggregate count or nothing at all until close. |
ToxicBiohazard
left a comment
ToxicBiohazard
left a comment
There was a problem hiding this comment.
Went through this end to end and pulled the branch down to run it. Tests pass locally (87) and CI is green.
I spot checked the trickier claims in 700467d rather than taking them on faith, and they hold up. The ViewStore.dispatch race is real, followup.send does force wait=True for application webhooks so BallotView.message gets set and on_timeout actually fires, and the asyncmy dialect does set CLIENT_FOUND_ROWS. The 840s ballot timeout is a good catch too, since py-cord would otherwise clamp it to exactly 900 and the cleanup edit runs on the interaction token.
Comments below are mostly what is left after that commit, and they lean operational rather than logic. The two I would want sorted before this merges are the results going missing once the close is claimed, and the poll posting to whatever channel the command was run in. The rest are take or leave.
Holding off on the two design questions from your comment for now, I will come back to those separately.
| result = await session.execute( | ||
| update(AnonymousVoteSession) | ||
| .where(AnonymousVoteSession.id == session_id, AnonymousVoteSession.closed.is_(False)) | ||
| .values(closed=True) | ||
| ) | ||
| if result.rowcount == 0: | ||
| logger.debug("Anonymous vote session %s is already closed or gone.", session_id) | ||
| return None | ||
| await session.commit() |
There was a problem hiding this comment.
The conditional UPDATE fixes the double publish, but it also makes closed = True durable before anything has actually been published. Everything after this point is best effort and outside the transaction, so there are a few ways the tallies go missing for good:
_resolve_poll_channelreturns None (channel deleted, permissions changed, transient 5xx onfetch_channel) andclose_anonymous_votejust returns.- Both the edit and the fallback send fail, which is the case your own log line calls unrecoverable.
- The bot restarts between this commit and the send.
register_anonymous_vote_viewsfilters onclosed.is_(False), so nothing ever picks it back up.
The ballots are all still sitting in the table, so it is recoverable with SQL but not through the bot, which is the awkward part when a nomination vote people waited a week on comes back empty.
Could we split "voting is over" from "results are out"? A published_at column would let you claim the close exactly as you do here, then have register_anonymous_vote_views also sweep closed.is_(True) & published_at.is_(None) on startup and retry the publish. If that is too much for this PR, even a small /admin vote-results <session_id> that re-renders build_results_embed from the stored ballots would give us a way out without pulling in someone with database access.
| async def _refresh_poll_message(self, poll_embed: discord.Embed | None) -> None: | ||
| """Redraw the public poll message so the new activity box shows.""" | ||
| if poll_embed is None or self.poll_message is None: | ||
| return | ||
| try: | ||
| await self.poll_message.edit(embed=poll_embed) | ||
| except discord.HTTPException: | ||
| logger.exception("Failed to refresh poll embed for session %s after vote.", self.session_id) |
There was a problem hiding this comment.
Two things about refreshing on every ballot.
The first is the participation leak you already flagged in the description, but I think calling it a count undersells it. Because this fires the instant a ballot lands, it is a live feed rather than an aggregate. Anyone sitting in the channel sees a box appear next to a specific nominee at a specific second, and in a mod team of five or ten, matching that against who is online right then narrows it a long way. The approve/reject split staying hidden does not help much once you can tell who participated and when.
Second, per message edits are limited to roughly 5 per 5 seconds. If the team gets told "vote now" and a dozen people go at once, these edits queue behind the bucket and the ephemeral confirmations start lagging the clicks.
Batching this behind a short timer, or dropping the live boxes and only rendering counts at close, would deal with both at once.
| for vote_session in open_sessions: | ||
| bot.add_view(AnonymousVoteView(vote_session.id, bot, vote_session.candidates)) | ||
| if vote_session.closes_at <= now: | ||
| bot.loop.create_task(close_anonymous_vote(bot, vote_session.id)) | ||
| else: | ||
| schedule_vote_close(bot, vote_session.id, vote_session.closes_at) |
There was a problem hiding this comment.
_register_persistent_views runs from on_ready, which fires on every full gateway reconnect and not just at startup, so each reconnect adds another sleeping schedule() task for every open session. Your conditional UPDATE in _close_vote_session means only one of them can publish, so this is not a correctness problem anymore, but they do pile up for the lifetime of the vote and then all wake at the same moment and hit the database together.
On a 12 hour vote nobody would ever notice. With a 30 day ceiling it is a lot more visible. Keeping a set[int] of session ids already scheduled, or holding the tasks and cancelling the previous one before rescheduling, would be enough.
Worth saying register_ban_views has the same shape, so this is an existing pattern rather than something this PR invented. The long vote window is just what makes it show.
| values = interaction.data.get("values", []) | ||
| if not values: | ||
| await interaction.response.send_message("No nominee selected.", ephemeral=True) | ||
| return | ||
|
|
||
| await interaction.response.defer(ephemeral=True) | ||
| candidate_id = int(values[0]) |
There was a problem hiding this comment.
int(values[0]) is unguarded. Discord validates select values against the component it sent, so in practice this holds, but if it ever does not the failure mode is a ValueError raised after the defer has already gone out. The voter gets silence and we only find out from the logs.
interaction.data is typed dict | None as well, so (interaction.data or {}).get("values", []) covers both. A try/except ValueError falling through to the existing "Unknown nominee." path would be plenty.
|
|
||
| def _format_nominee_line(candidate: AnonymousVoteCandidate, vote_count: int) -> str: | ||
| """Format a nominee line with one activity box per cast ballot.""" | ||
| line = f"• **{candidate.display_name}** (`{candidate.user_id}`)" |
There was a problem hiding this comment.
display_name goes into the embed description unescaped, and embed descriptions do render masked links. A nominee whose nickname is something like [click](https://example.com) ends up as a live link inside a staff poll, and ** or a backtick in a nickname will quietly mangle the line.
Admins pick the nominees so the blast radius is small, but discord.utils.escape_markdown(candidate.display_name) here costs nothing. Same thought for wherever display_name reaches build_results_embed.
| # MariaDB reports 2 from ON DUPLICATE KEY UPDATE only when the stored value actually | ||
| # changed, and documents 0 for a no-op update. We never see that 0: SQLAlchemy's MySQL | ||
| # dialect ORs CLIENT_FOUND_ROWS into client_flag to get supports_sane_rowcount, so the | ||
| # server counts rows matched and 1 means "inserted or re-voted the same way". | ||
| UPSERT_ROWCOUNT_UPDATED = 2 |
There was a problem hiding this comment.
I went and checked this one because it is the sort of claim that is easy to get backwards, and it is right. MySQLDialect_asyncmy overrides _found_rows_client_flag to return CLIENT.FOUND_ROWS unconditionally, so the flag really is always set for our connection string and the documented 0 is unreachable. Good comment to have left behind.
One consequence worth knowing about: because a no-op update also reports 1, someone re-picking the choice they already had gets "Vote recorded" rather than "Vote updated". Harmless, just slightly odd if you spot it, and not worth a second round trip to fix.
| view = AnonymousVoteView(session_id, self.bot, candidates) | ||
| self.bot.add_view(view) | ||
| try: | ||
| message = await ctx.channel.send(embed=poll_embed, view=view) |
There was a problem hiding this comment.
This posts to whatever channel the command happened to be run in, with no allowlist and no confirmation step in front of it. One wrong channel and the nominee names, the live activity boxes and the final approve/reject tallies are all readable by everyone who can see that channel, nominees included.
For the CC program that feels like the worst possible version of a typo, since a rejected nominee would find out in public. Could we either pin the poll to a configured staff channel, or check ctx.channel.id against the relevant settings.channels entries before we get this far? Fine to leave as is if the intent is that the starter always picks deliberately, but right now nothing stops it.
| await _delete_vote_session(session_id) | ||
| return await ctx.followup.send("Could not post the poll in this channel.", ephemeral=True) | ||
|
|
||
| await _attach_poll_message(session_id, message.id) |
There was a problem hiding this comment.
The rollback above covers the send failing, which was the important half. There is still a window here though: if the bot dies between ctx.channel.send returning and this update committing, the poll is live in the channel while the row keeps message_id = NULL. On restart the session is picked up as open, and at close _edit_poll_with_results bails on if not message_id and posts results as a fresh message, leaving the original poll sitting there with controls that still look usable.
That degrades reasonably rather than breaking, so I would not hold the PR on it. Worth a short comment in the code though, so the next person does not read message_id as always populated for a poll that got posted.
| for user_id in member_ids: | ||
| member = await _fetch_member(ctx, user_id) |
There was a problem hiding this comment.
Worst case this is 25 sequential round trips, which is exactly why you added the defer, so no correctness issue. Just noting that guild.query_members(user_ids=member_ids) does the same lookup in a single gateway call and caps at 100 ids, so it would collapse the loop and be gentler on the member endpoint if you feel like it.
|
|
||
|
|
||
| class AnonymousVoteBallot(Base): | ||
| """A single voter's choice for one nominee. voter_id is never shown in Discord.""" |
There was a problem hiding this comment.
Worth being precise in this docstring, because "never shown in Discord" is carrying a lot of weight. voter_id and choice sit next to each other in plain text under a unique constraint, so anyone with read access to the database or to a backup can reconstruct exactly who voted which way on every nominee.
I do not think that is avoidable when you need the pair to support changing a vote, and I am not asking for hashing here. My concern is the gap between this and what the poll embed tells voters ("Votes stay anonymous"). Anonymous from the mod team and anonymous from whoever holds the database are quite different promises, and the people casting these votes should probably be told which one they are getting before this goes live.
3 participants
Summary
/admin votefor Administrator, Community Manager, and Community Team to start a timed anonymous poll over multiple Discord membersTest plan
alembic upgrade head)/admin voteappears for Admin/CM/CT only2m)Made with Cursor