Skip to content

Implement challenge rebuilding in API v3 - #1310

Open
jake-low wants to merge 1 commit into
jlow/api-v3-authfrom
jlow/api-v3-challenge-rebuilding
Open

jake-low wants to merge 1 commit into
jlow/api-v3-authfrom
jlow/api-v3-challenge-rebuilding

Conversation

@jake-low

@jake-low jake-low commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Builds on top of #1307

This implements a new PUT /api/v3/challenges/:id/tasks endpoint. The endpoint accepts a newline-delimited stream of GeoJSON Features, each one representing a task, and upserts those tasks into the challenge. You can call the endpoint after creating a challenge to populate it with tasks, or at any time later to update the existing tasks, remove old ones, add new ones, etc.

The GeoJSON features are matched up with existing tasks by their id property (which the API calls the task's key elsewhere to distinguish it from the integer ID assigned by MapRoulette).

  • Features which don't match any existing task are inserted as new tasks.
  • Those that match existing tasks cause the task's definition to be updated; any task status added by mappers (fixed, not an issue, etc) is left unchanged.
  • Tasks which don't match any uploaded Feature are closed (given the special status 9). This is different from "Fixed"; it means the task was removed by the challenge manager.
  • Tasks which are currently closed but match a Feature in the upload are reopened (this is why we mark tasks as closed rather than simply deleting them).

The endpoint is atomic: the entire upsert either succeeds or fails as one operation. It validates GeoJSON tasks one at a time in a stream as they are uploaded, and spools the results to a temporary file. Once the entire stream is uploaded, it acquires a database connection and applies the update in a transaction.

This endpoint replaces the "local GeoJSON" challenge creation and update workflow from API v2. It does not support OverpassQL or remote GeoJSON challenge types. It also does not support cooperativeWork (Tag Fix or Cooperative challenge types).

PATs with the challenges:write scope can call this endpoint, so it can be used from third-party scripts.

@jake-low
jake-low added this pull request to stack #1308 November 6, 2026 20:49
@sonarqubecloud

sonarqubecloud Bot commented Nov 6, 2026

Copy link
Copy Markdown

@atiannicelli atiannicelli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this end to end against PostGIS with v2's evolutions applied. These all behaved correctly: created/updated/unchanged/closed/reopened counts, case-insensitive id matching, the 422 errors (line, id and pointer, for invalid geometry and duplicate ids), 415 for the wrong content type, gzip bodies, and the 403 and 404 cases. #1300 statement triggers kept completion_metrics and the dirty tile cells in sync, and a 50,000-feature (6.4 MB) upload applied in 4.4s.

One transaction, a per-challenge lock, and validating everything before writing is a solid design. Most of the issues below are places where it doesn't yet match what v2's rebuild does, plus one cross-service risk.

Must fix: v2's cache can undo these writes
v2 caches challenges and tasks in memory by default (caching.type="caffeine", with expireAfterAccess, so entries that keep being read never expire). ChallengeDAL.update merges the request over the cached challenge and writes the whole row back, falling back to the cached overpass_ql, remote_geo_json, status and status_message (ChallengeDAL.scala). TaskDAL.update does the same for a task's name, geometry, priority and status (TaskDAL.scala). So after a v3 upload, an edit through v2 can silently restore the old remote GeoJSON URL, challenge status or task data. This needs either v2 running with MR_CACHING_TYPE=noop in production (documented as a deploy requirement for v3), or a way for v3 to invalidate v2's cache.

Should fix

  • The upload SQL isn't checked. db/uploads.rs uses runtime sqlx::query, so unlike the query! macros it isn't verified against the schema at compile time, and there are no database tests. A few #[sqlx::test] cases (create, update, close, reopen, duplicate ids, invalid geometry) would cover the core path.
  • Deploys abort uploads. DRAIN_TIMEOUT in lib.rs is 5s, but an upload's apply phase can run for up to 120s, so a deploy cuts off any upload in progress. It rolls back cleanly, but the client sees a reset after sending up to 100 MB. Consider a longer drain for upload routes.

The rest are line comments.

Comment thread api-v3/src/db/uploads.rs
Ok(sqlx::query(
"INSERT INTO tasks
(name, parent_id, instruction, status, priority, geojson, geom, location)
SELECT s.key, $1, '', 0, $2, s.geojson, s.geom, ST_Centroid(s.geom)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must fix (or scope out explicitly): new tasks always get the challenge's default_priority ($2), and updated tasks keep whatever priority they had. v2 evaluates the challenge's high/medium/low priority rules for every task on each rebuild (Task.getTaskPriority, TaskDAL.scala; also updateTaskPriorities, ChallengeDAL.scala). Challenges that use priority rules will get wrong priorities after a v3 upload. Options: apply the rules here against the staged properties, reject challenges that have priority rules with a 409 like Overpass challenges, or document it as a known limitation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still up in the air, but most likely I will turn this into a known limitation (i.e., API v3 will initially ship without support for task priorities). Down the road, I'd rather implement this by having clients just upload a priority field on each task, rather than defining priorities as rules on the challenge which MapRoulette needs to evaluate on each task every time it's updated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I think either way works and if it is easier to not have the rules ont he server and just require a priority field then I'm down with that.

let open = sqlx::query_scalar!(
r#"
WITH open AS (
SELECT count(*) AS n FROM tasks WHERE parent_id = $1 AND status IN (0, 3, 6)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must fix: this counts too-hard (6) as open. v2 counts only created (0) and skipped (3) when it decides between finished and ready (updateFinishedStatus, updateReadyStatus). I reproduced it: with only a too-hard task left, this sets the challenge to ready (3) where v2 sets finished (5), so the status will flip whenever the other service next touches the challenge. Using IN (0, 3) here would match v2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API v2 is inconsistent about whether or not tasks marked "Too Hard" (status 6) are counted as open or not. The mapping workflow treats them as open (GET /tasks/random will return them, they're included in the task map on the "Explore" page, and they can be bundled with other open tasks when mapping). But the challenge management endpoints treat "Too Hard" as a resolution, so a challenge with all of its tasks marked "Too Hard" is considered finished. This status also has two different names in the UI ("Can't complete" is the other one) which contributes to this confusion.

Making API v3 treat status 6 as open is a deliberate attempt to make this more consistent, at least until we can overhaul statuses at some later time (there are several others that have inconsistent meaning or are redundant)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - awesome. that makes sense.

Comment thread api-v3/src/db/uploads.rs
Ok(sqlx::query(
"UPDATE tasks t SET status = 9
WHERE t.id IN (SELECT id FROM upload_locked)
AND t.parent_id = $1 AND t.status IN (0, 3, 6)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must fix (same issue): this also closes too-hard tasks that are missing from the upload (and line 185 locks them). v2's removeIncompleteTasks only touches created and skipped tasks (ChallengeDAL.scala).

Consider, separately: v2 deletes those tasks, while this disables them (status 9). That's a reasonable choice since it keeps history and allows reopening, but disabled tasks pile up and count in the totals. A 50,000-feature upload followed by a one-feature upload left over 50,000 disabled tasks on the challenge. Worth documenting, or giving owners a way to purge them.

-- this challenge into what v2 calls a "local geojson challenge"
-- (one created by direct upload)
overpass_ql = NULL,
remote_geo_json = NULL,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must fix, or document: this silently turns a remote-GeoJSON challenge into a local one (I confirmed remote_geo_json is NULL after an upload). The description says remote GeoJSON isn't supported, but the endpoint accepts these challenges and wipes the URL, so the owner loses the source and v2's rebuild-from-URL stops working. Either reject them with a 409 like Overpass challenges, or make the conversion explicit in the docs and response.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah, that's an inconsistency in behaviour between Overpass QL and Remote GeoJSON challenges.

My goal is ultimately to deprecate Overpass QL and Remote GeoJSON challenge support in the server API, since both can be implemented on the client (for example, a client can easily fetch data from a remote URL and then upload that to the MapRoulette API). Overpass QL challenges would still be supported in the maproulette.org UI, but the query would be run from the browser. This simplifies the backend implementation and improves security (fetching and processing user-supplied URLs is a can of worms).

So the intent with the new PUT /api/v3/challenges/:id/tasks endpoint is that it converts legacy challenge types to the new form (what v2 calls "Local GeoJSON" challenges, i.e. where tasks are created directly from GeoJSON features uploaded by the user).

I added the HTTP 409 response for Overpass QL challenges because I realized that existing Overpass QL challenges use an integer ID for their task key, which does not uniquely identify the OSM element that the task is linked to. For example, a task might have ID 12345 if it corresponds to way/12345. API v3 expects tasks which are linked to OSM elements to provide keys in the latter form. So before I let the v3 endpoint "take over" Overpass QL challenges, I need to write a migration to fix existing challenges' IDs, otherwise the first time someone uses this endpoint to update a challenge that was created using Overpass, it'll close all of that challenge's open tasks and create new ones, instead of properly matching them.

Remote GeoJSON challenges don't have the same issue so I left them enabled in the PR as it currently stands, though I see now that the wording in the PR description I wrote (below) contradicts this:

This endpoint replaces the "local GeoJSON" challenge creation and update workflow from API v2. It does not support OverpassQL or remote GeoJSON challenge types.

What I should have said was that this endpoint does not intend to re-implement API v2's Overpass QL or Remote GeoJSON challenge types; in other words, uploading tasks by HTTP PUT with an NDJSON body will be the only way to "rebuild" a challenge in API v3. Users who want to can convert their challenges which were created under API v2 to this form by calling the API v3 endpoint.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... that's a bummer. We use the remove GeoJson challenges all the time. When updating a challenge every week like we do it is much easier to:

  1. push data to geoJson file that in publicly available
  2. ping the challenge to rebuild the challenge

I can see your point that I can just implement an update process instead of a challenge rebuild API call, but the issue is that with lots of tasks the update can take a while and keep our client busy for that whole time.

If you must then you must, but I personally like the remote geoJson functionality.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rebuild operation should be a lot faster through the new API (a couple of seconds to rebuild a challenge with thousands of tasks, rather than several minutes) so that should hopefully help alleviate some of the pain.

Comment thread api-v3/src/db/uploads.rs
let (updated, reopened): (i64, i64) = sqlx::query_as(
"WITH changed AS (
UPDATE tasks t SET
status = CASE WHEN c.reopen THEN 0 ELSE t.status END,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix: reopening only sets status = 0. When v2 resets a task to created it also clears completed_time_spent, completed_by, completion_responses, is_bundle_primary, bundle_id and mapped_on (TaskDAL.scala). I confirmed a reopened task kept completed_by and mapped_on. v2's rebuild upsert also sets changeset_id = -1 and deletes task_review rows for tasks back at created (TaskDAL.scala). Bundled tasks aren't considered when closing either.

max_errors: usize,
) -> Result<UploadCounts, UploadError> {
db::uploads::raise_statement_timeout(conn).await?;
db::challenges::lock_upload(&mut *conn, challenge).await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix: the per-challenge lock is taken only after the whole body has been received, and waits at most the session's lock_timeout (5s). A second upload to the same challenge (a retry, or another v3 instance) that arrives while one is applying gets a 503 after sending up to 100 MB. Consider checking the lock before receiving (e.g. pg_try_advisory_lock), or raising lock_timeout for this transaction to match the 120s statement timeout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the lock timeout fairly short because the request holds a database connection while it's waiting, so if it is very long it could be used as a denial of service vector. If a challenge owner sends two simultaneous requests to rebuild their challenge, I think it's fine if one of those requests fails (the backend doesn't need to queue and serialize the second request for them)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Comment thread api-v3/src/config.rs
fn default() -> UploadLimits {
UploadLimits {
max_bytes: 100 * 1024 * 1024,
max_features: 100_000,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix: v2 caps challenges at 50,000 tasks (max_tasks_per_challenge, application.conf). Should this match, and be configurable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bumped it because there are existing challenges which have >50,000 tasks somehow (I'd guess due to a bug where the limit is not being enforced in some rebuild path, but I'm not sure), and updating those challenges through API v3 would be impossible if the same limit were kept.

Configurability probably wouldn't hurt, though in practice there are very few third-party instances of MapRoulette and I don't know if any of their operators care about changing this particular limit.

Comment on lines +115 to +118
None => {
problems.push(problem(
"/id",
"is required, because it identifies the task",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: v2 names tasks from the challenge's osmIdProperty, then id, @id, osmid, osm_id or name, falling back to a random UUID (ChallengeProvider.scala). Requiring a top-level id is cleaner, but the first v3 upload to a challenge whose tasks were named from properties or UUIDs will close every existing task and create new ones, so progress on them doesn't carry over. Worth calling out in the docs, or matching on osmIdProperty when the challenge has one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deliberate. I plan to write up a migration guide for challenge authors once this is deployed that describes the changes they need to make when producing challenge data in order to safely switch to the new API endpoint.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK., This is fine with me. the change it fine. We always delete unfinished tasks before we rebuild. So a new task is fine... the only thing that worries me is that is we have issues that someone marked as "not an issue" but we keep it in the dataset then it will go from being not an issue to a new issue with a new name that hasn't yet been completed? hmmm.

@jake-low jake-low Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only thing that worries me is that is we have issues that someone marked as "not an issue" but we keep it in the dataset then it will go from being not an issue to a new issue with a new name that hasn't yet been completed?

If you keep the same key (i.e. the GeoJSON Feature's id), then a task that's marked as "not an issue" should keep that status even if you include it again in a subsequent PUT. Conceptually, the idea is that the endpoint updates task definitions (geometry, properties, in the future possibly priority, etc) but never changes the statuses that mappers set on the tasks. So hopefully if I've understood you correctly, this means it does what you hope it does by default.

The only "exception" is closed (status = 9) which isn't really a status; it should be thought of as more like a soft delete. Refactoring the current status concept in the future (possibly splitting it into status, i.e. is the task open or not, and resolution, i.e. what choice did the mapper make when they closed it) might help make this make more sense.

Comment thread api-v3/src/db/uploads.rs
SELECT s.line, s.key, '/geometry',
'is invalid: ' || v.reason || coalesce(' at ' || ST_AsText(v.location), '')
FROM upload_staged s
CROSS JOIN LATERAL ST_IsValidDetail(s.geom) v

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: v2 repairs invalid geometry with ST_MakeValid (130.sql) and skips features it can't parse, while this rejects the whole upload. Strict is defensible for a new API, but scripts moving from v2 will hit it (e.g. self-intersecting polygons), so it's worth mentioning in the endpoint docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto above: this is a deliberate change to make the API stricter and reject invalid data rather than silently coercing it into shape.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

@ljdelight

ljdelight commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

My robot and I have been looking at this and have found two bigger things, pasting below. It tracked insert times and it all looks good, some additional findings are what Alex had mentioned and I removed those from the below. Just two new ones.

How I tested. I ran v2 (sbt run, dev mode) and v3 (release build) against the same local Postgres. I created and then rebuilt a challenge on each, using the same 20k-feature line-by-line file. The rebuild input was 19,300 features: 1,000 dropped, 500 with changed properties and 300 new.

  • v2: PUT /api/v2/challenge/:id/addFileTasks?lineByLine=true&skipSnapshot=true&removeUnmatched=true
  • v3: PUT /api/v3/challenges/:id/tasks

I counted every table (81) before and after each step, recorded the SQL each server logged, and diffed the tasks through the DB and through GET /api/v2/challenge/:id/tasks.

Timings (3 runs each)

v2 (to READY) v3 (synchronous 200)
Create, 20,000 tasks 2.60–2.99 s 1.53–1.58 s
Rebuild 2.96–4.47 s 1.00–1.16 s
Re-upload of an unchanged file 3.14 s (rewrites every row) 0.98 s (20,000 unchanged, no writes)

The response counts were correct in every run: created 300, updated 500, unchanged 18,500, closed 1,000. Reopening the 1,000 closed tasks also worked.

Must fix

  1. Floats are stored one double-precision step off. serde_json is built without float_roundtrip, so its parser sometimes picks the neighbouring float64. About half the tasks had a coordinate or property value off, for example normalized_volume stored as 0.9812036944259435 → …436. With serde_json = { features = ["float_roundtrip"] } and the same file, 0 of 20,000 tasks differed from v2.
  2. Timestamps are 6 hours off from v2. now() lands in timestamp without time zone columns (tasks.created/modified, challenges.modified/data_origin_date). The v3 session writes UTC and v2's writes the JVM's local zone, so through the v2 API every v3 task shows created 6 hours in the future. This only goes away if prod v2 also runs in UTC. Either make it explicit (set the session TimeZone) or confirm prod v2 runs in UTC.

Nits

  • v3 adds "id" to each stored feature, and v2's line-by-line path doesn't. So the first v3 upload over an existing v2 challenge counts every task as updated.
  • Lines starting with an RFC 7464 record separator (0x1E) fail with "not valid JSON" plus a hint about FeatureCollections. v2 strips that character. Either strip it or make the error message name it.
  • data_origin_date is overwritten on every upload. v3's finish_upload sets it to now(). v2 keeps the existing value unless dataOriginDate is passed.
  • The stored geometry type differs. v2's task_geometry_from_geojson collects every geometry into a multi or collection type (ST_Collect) and runs ST_MakeValid on each. v3 stores what was uploaded with ST_Force2D (LineString stays LineString). The values match, but anything that checks GeometryType or relies on ST_MakeValid repairs will behave differently.

@ljdelight

Copy link
Copy Markdown
Contributor

And of course i did a stress to check bounds, looks good! The robot noticed a couple of minor things. I do wodner if 2 apply threads will be enough for a prod environment, maybe the proxy logs will have data for something like that (but there are a ton of ways to add tasks under the v2 api so, yeah, tricky to know for sure).

Anyway, stress result! (so far i only did a 100% write, no mixed stuff (obviously because there's no read yet)):

Stress test: concurrent challenge creates (PUT /api/v3/challenges/:id/tasks)

How I tested. I created a fresh challenge for each upload through v2 (not timed). Then I fired 1, 4, 8 and 16 v3 uploads at the same time, all starting within 20 ms of each other. Each upload had 20,000 features shifted to its own non-overlapping location on a 4×4 lon/lat grid. Both servers used the same local Postgres. I sampled pg_stat_activity every ~0.3 s, and afterwards checked every challenge in the DB.

Concurrent HTTP results Fastest / slowest upload Last one done Tasks/s overall v3 DB connections (max / busy)
1 1 × 200 1.49 s 1.53 s ~13k 3 / 1
4 4 × 200 2.04 / 3.88 s 3.92 s ~20k 6 / 2
8 8 × 200 1.93 / 7.12 s 7.16 s ~22k 10 / 2
16 8 × 200, 8 × 503 1.98 / 7.22 s (the 200s) 7.42 s ~22k 10 / 2

What I checked (all 29 challenges, 0 mismatches)

  • Each of the 21 accepted uploads has exactly 20,000 open tasks, status READY (3), and a bounding box centred on its own location, so nothing landed in the wrong challenge.
  • Each of the 8 rejected (503) challenges is untouched: no tasks, NULL status, NULL bounding.
  • No duplicate task names within any challenge.
  • No v3 errors other than the 8 expected rejections.

What this shows

  • Uploads apply two at a time. Completion times come in pairs (about 2.0, 3.6, 5.4 and 7.1 s at 8 concurrent). That's applying = 2: two uploads write at once and the rest queue in memory without holding a DB connection. Overall throughput levels off at about 22k tasks/s, and the slowest upload's time grows with roughly N/2 × 1.8 s.
  • Beyond 8, uploads are refused, not queued. At 16, exactly 8 got 503 with Retry-After: 5 and an application/problem+json body, within about 20 ms. Clients have to retry those themselves.
  • The connection pool doesn't run out. v3 reached its 10-connection maximum, but at most 2 were ever busy. There were no pool-acquire timeouts, and the whole database peaked at 50 of its 100 connections.

Suggestions

  1. Log the 503 rejections at WARN or INFO instead of ERROR. They currently log as ERROR request failed … too many uploads are being received, but this is intended load shedding. At ERROR, a burst of uploads looks like an outage in alerting.
  2. Slow-statement warnings under normal load. With two uploads applying at once, INSERT INTO tasks took 1.00–1.37 s and set off sqlx's default 1 s slow-statement warning 13 times. Raise the threshold for the upload transaction, or expect this noise in production.
  3. Make the concurrency limits configurable. receiving: 8 and applying: 2 are hard-coded in config.rs; the code already notes they could come from env vars. Under load, applying is what sets the latency, so it's the one most worth exposing.
  4. Rejected uploads leave an empty challenge. After a 503 the challenge has no tasks and a NULL status, and v2's UI may show it as "not built" rather than failed. Document that clients must retry on 503, or have the frontend handle it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants