Move challenge reports from github to database - #1270
Merged
Merged
Conversation
CollinBeczak
changed the title
CollinBeczak
force-pushed
the
collin/challenge-reports
branch
from
September 8, 2026 17:33
eaed077 to
5492f9a
Compare
A keyword names a tag on the *challenge*, but the queries matched it by joining `tags_on_challenges` into a per-task query. That returns every task of a challenge once per requested tag the challenge carries, so anything downstream that counts or clusters rows sees the same task several times. Demonstrated on a 166k-task database by giving an eligible challenge a second real keyword and asking for both: the join returns 22,430 rows for 11,215 tasks, the semi-join 11,215. Any challenge tagged with two of the selected categories is enough, which is ordinary -- challenges routinely carry several keywords and the explore UI lets more than one be selected. What the duplication reached: - Cluster counts on the keyword-filtered explore map were inflated, and cluster centroids were pulled toward multi-tagged challenges, since the duplicate rows inflate the centroid weight as well as the count. - At zoom 12, where features are grouped by location, a task counted twice reported 2 and rendered as an overlap stack rather than a single clickable task, with its id repeated in `task_ids_str`. - queryTaskMarkersWithOverlaps returned each task once per matching tag, and since it groups by location with DBSCAN, the duplicates of one task became a phantom overlap group. exploreChallenges had the same join, hidden behind a SELECT DISTINCT. It moves to the semi-join and drops the DISTINCT, which nothing else needs: the only other join there is many-to-one on the parent project. That also lifts a restriction the DISTINCT imposed -- Postgres requires every ORDER BY expression to appear in the select list under SELECT DISTINCT, so a sort could not order by an expression. The other three keyword call sites in TaskClusterRepository were already protected by SELECT DISTINCT / COUNT(DISTINCT), so they were correct; they move to the semi-join too, to leave one spelling of the question in the file. This is a correctness fix, not a performance one -- the two forms measure the same. Best of 3 at zoom 0 on that database: 9.19ms vs 9.17ms for a keyword matching one challenge of 31, and 38.0ms vs 36.0ms where the join was duplicating every row. Cost there is dominated by the scan over tasks, which neither form avoids at low zoom. Not covered: ProjectRepository.getSearchedClusteredPoints joins the tag tables the same way with no DISTINCT, so it can still return a challenge's clustered point once per matching tag.
A reviewer claiming a bundle locked each member task individually, which left one `locked` row per task. Locking.lockBundle already models a bundle as one row on the primary task with the members in `bundled_tasks`, so the two representations disagreed and the singleOpt lookups in resolveLockHolder/resolveLockBundle could see two rows covering the same task. - claimTaskReview now takes the whole list through lockBundle with reviewClaim = true, so one row covers the bundle. Review claims stay exempt from the one-lock-per-user invariant, which is what the new reviewClaim parameter on lockBundle carries through. - lockBundle checks every task the row will cover, not just the primary, and folds any of our own overlapping rows into that one row. Without this a claim over tasks already covered by another of our rows would leave two rows covering the same task. - claimTaskReview drops leftover rows from a previous claim. The unclaim it runs first only clears task_review, so those rows accumulated and kept their tasks locked. - unclaimTaskReview clears the claim on every member of the bundle, since releasing the single row releases them all, and runs its unlock inside the surrounding transaction. - A lock conflict response now carries parentId alongside parentName, so a client can link to the challenge holding the conflicting lock.
Tile clusters were the grid bins themselves, so a cluster's position was its cell centroid and its size was fixed by the grid. Neighbouring cells holding a handful of tasks each stayed separate markers no matter how far apart their tasks actually were. The repository now runs k-means over the cells feeding a tile and returns the resulting clusters, so markers land on where the tasks are and nearby cells merge into one cluster instead of sitting side by side. A separation pass then collapses centroids closer together than 25 screen pixels, which is what makes zooming out consolidate clusters: k-means returns exactly k clusters however tightly packed its input is, so k is only a ceiling. Evolution 121 coarsens the grid to match: CELL_BITS 4 -> 3, moving the leaf level from slippy zoom 15 to 14 so a display tile holds 8x8 = 64 cells instead of 16x16 = 256, each cell twice as wide. Only the cell<->slippy-zoom mapping changes -- the roll-up is untouched -- so the leaf functions from evolution 107 are redefined at zoom 14 and the pyramid is rebuilt. The Downs restores the zoom 15 leaf and rebuilds again. Cluster weight is the filtered count, not the cell total. A cell's stored sums cover every task in it, so its centroid is the right position either way, but carrying the total into the merge let a cell drag a cluster in proportion to the tasks the filter had just excluded -- a marker reporting a handful of expert tasks could sit on top of a thousand easy ones. This did not arise before, when every cell was its own marker and a mis-weighting could not cross cell boundaries. Measured against the base tables on a 166k-task database under a difficulty filter, mean cluster displacement drops from 563m to 2m at z=8 (worst 2345m -> 4m) and from 68m to 3m at z=11 (worst 298m -> 12m). A new `clusterMarkers` shares the whole src -> k-means -> separation chain with the MVT paths and returns the markers as plain numbers, so a test can assert where one landed without decoding protobuf. The repeated-request test now seeds more cells than MAX_CLUSTERS, since at or below the ceiling k-means returns one cluster per input and the clustering step is an identity.
CollinBeczak
force-pushed
the
collin/challenge-reports
branch
from
September 8, 2026 17:59
5492f9a to
c23ebcd
Compare
Tile clusters were the grid bins themselves, so a cluster's position was its cell centroid and its size was fixed by the grid. Neighbouring cells holding a handful of tasks each stayed separate markers no matter how far apart their tasks actually were, and dense regions came out looking like graph paper. The pyramid becomes the *input* to clustering rather than the output. A tile reads micro-aggregates from a level below its display zoom, runs k-means over them in Web Mercator, then merges any centroids closer together than 25 screen pixels. Markers follow the data instead of the grid, and cluster extent adapts to local density. The merge is what makes zooming out behave: k-means returns exactly k clusters however tightly packed its input is, so k is only a ceiling and consolidation comes from the separation pass. Read depth is DETAIL_BITS = 2 rather than coarsening the grid. Both would bound the k-means input at 4096 cells per tile, but the pyramid stops at MAX_CELL_ZOOM, so near the top the display zoom eats into the depth available and a tile falls back on CELL_BITS alone. Leaving CELL_BITS at 4 keeps z=11 at 256 cells of 16px, where k-means still has more input than MAX_CLUSTERS to partition and the cells are finer than the merge distance. Coarsening to CELL_BITS = 3 would have left 64 cells of 32px there: k equal to the input size makes k-means an identity, and cells wider than the merge distance make the separation pass a no-op, putting the top cluster zoom back to one marker per grid cell. Tuning the read depth instead needs no migration and no pyramid rebuild -- level z+2 already exists. Cluster weight is the filtered count, not the cell total. A cell's stored sums cover every task in it, so its centroid is the right position either way, but carrying the total into the merge let a cell drag a cluster in proportion to the tasks the filter had just excluded -- a marker reporting a handful of expert tasks could sit on top of a thousand easy ones. This did not arise before, when every cell was its own marker and a mis-weighting could not cross cell boundaries. Measured against the base tables on a 166k-task database under a difficulty filter, mean cluster displacement drops from 563m to 2m at z=8 (worst 2345m -> 4m) and from 68m to 3m at z=11 (worst 298m -> 12m). A new `clusterMarkers` shares the whole src -> k-means -> separation chain with the MVT paths and returns the markers as plain numbers, so a test can assert where one landed without decoding protobuf. The repeated-request test seeds more cells than MAX_CLUSTERS, since at or below the ceiling k-means returns one cluster per input and the clustering step is an identity.
A paused challenge's tasks cannot be locked, completed or reviewed until it is resumed, and a finished challenge has no tasks left at all. Both still showed up as available work. - Paused challenges drop out of all four tile paths: this repository's live MVT queries, and the cached pyramid's rebuild_leaf_cell and rebuild_all_tile_cells (evolution 122). That evolution also widens the challenge dirty-marking trigger from evolution 107 to fire on `paused`, without which a pause never reaches the pyramid, and marks the cells of already-paused challenges stale so the scheduled drain recomputes them rather than rebuilding everything. - Paused challenges also drop out of the task cluster queries and out of exploreChallenges, which additionally omits STATUS_FINISHED challenges. NULL status predates the column and counts as unfinished. - Locking a task or a bundle in a paused challenge is rejected: there is no work to hold a lock for. - Evolution 121 reconciles challenges left at READY while showing 100% complete. updateFinishedStatus keeps status in sync as tasks are worked, but paths that never run it (bulk status changes, deletions, restores) could leave a done challenge listed as work.
exploreChallenges filtered on challenges.bounding, the envelope of every task in a challenge. A challenge with tasks on two continents overlaps nearly any box, so searching Wichita returned USA-wide challenges. - The envelope is now only an index prefilter. A challenge matches when it has a task inside the requested area, checked with an EXISTS over tasks. - The POST form of the route takes a GeoJSON Polygon/MultiPolygon as `polygon` in the body and ANDs it with `bounds` on the same task, so a match has to be both in view and inside the place rather than in either. The geometry travels in the body because a city boundary from Nominatim runs to tens of kilobytes, past any practical URL length; the route parses with its own 2MB limit and rejects a non-polygon geometry with a 400. - `global` now defaults to false, matching taskTilesMvt and the UI toggle, which rendered "off" while the list still included global challenges. - sortBy accepts featured, tag_fix and cooperative. These group a kind of challenge to the front rather than ordering by the column, so each falls back to name to keep the ordering inside a group stable across pages.
Challenges had no card image of their own. Teams can now supply one, kept under review so an image on a public challenge card has been looked at. An image is owned by a team and moderated: any active member uploads one as a request (evolution 124 stores the bytes), a superuser approves or rejects it, and from then on any member of that team can attach it to their challenges through the new `teamImageId` on a challenge. - Attaching is validated on create and update. Image ids are plain numbers on the wire, so without this anyone could borrow another team's image, or one still awaiting review, by guessing an id. - An update that omits `teamImageId` leaves the current image alone and an explicit null detaches it, so a save that never touched the image picker cannot silently clear it. - Challenge json carries `teamImageId` plus a derived `avatarUrl`, keeping url construction in one place rather than in each client. - An unapproved image is served to the people with a reason to see it -- a superuser working the review queue, and members of the owning team -- as private/no-cache. To everyone else it stays a 404. Approved images are served anonymously with an ETag, since the url feeds plain img tags.
A team's avatar could only be a url pointing somewhere else on the internet. POST and DELETE /team/:teamId/avatar now store and clear an avatar we host (evolution 125), pointing the team's avatar_url at bytes of our own so the rest of the app keeps treating an avatar as a plain url. GET /team/:teamId/avatar/file serves them anonymously with an ETag, since that url feeds plain img tags. Unlike team images these are not moderated: a team admin could already point avatar_url at any image on the internet, so requiring review for the uploaded case would gate the safer of the two paths. The size and content-type limits are shared with team images. Stored keyed by team, so a team has at most one avatar and uploading a new one replaces the bytes rather than accumulating them. The bytes and the avatar_url update commit together -- updateTeam and updateGroup now accept the caller's connection -- so a failure cannot leave the team pointing at an avatar that was never stored.
A challenge report is a complaint about a challenge's design -- "this challenge is poorly designed and is causing incorrect edits" -- as opposed to a bug or a feature request. They were filed as issues in a public GitHub repo, which meant shipping a write-scoped GitHub token to the browser and publishing the reporter's identity alongside the complaint. They live in the database instead (evolution 126), so triage happens inside MapRoulette and the reporter's contact details stay private. Reports are submitted against a challenge, listed and worked through by the people entitled to see them, and the route file is registered ahead of challenge.api so the literal /challenge/reports and /challenge/report/:id paths are not swallowed by that file's GET /challenge/:id.
CollinBeczak
force-pushed
the
collin/challenge-reports
branch
from
September 8, 2026 18:54
c23ebcd to
377afd3
Compare
Main squash-merged the team images (#1272) and team avatars (#1280) work this branch was sitting on top of, so its own copies of both collided with the shipped versions rather than merging with them. Every one of those conflicts resolves to main's side: the branch carried the pre-review originals, and main has the evolved versions, including the avatar service and shared image mixin that landed with #1280. The challenge reports evolution moves from 125 to 126, since main's 125 is now the team_avatars table. The only genuine merges were the three test registries where both sides had added an entry - the master suite, the helper's tags and TestSpec's service mocks - which take both.
|
CollinBeczak
marked this pull request as ready for review
September 15, 2026 19:49
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.
Split from #1268
dependent on: #1271