Add the discovery v2 ingestor and tick workers - #2129
Conversation
One funnel for connector-reported discovery data. A drain page stages its items under the run row's lock and advances last_applied_sequence by the highest item sequence the page actually carried -- never by the response's run-wide highestSequence, which counts items the connector has produced but not handed over, and would skip everything in between. Advisory events stay advisory: progress is a cosmetic snapshot, an error joins the run's message log, and the two events meaning "there is something to fetch" schedule the tick that fetches it authoritatively. None of them commits run state or moves the cursor, and none may put work back on a run that has already finished. Certificates keep the v1 staging table, whose write is split into a REQUIRED variant so staged rows and the cursor accounting for them commit together; every other resource lands in discovery_item, deduped on the connector's uniqueRef.
run_meta holds the connector's opaque run handle -- a serialized List<MetadataAttribute>, the authority-v3 handle type -- which the lifecycle calls replay verbatim. The field was typed Map<String, Object>, which cannot represent a JSON array at all, so the first caller to actually replay the handle could not read one back. The mapper proof moves to discovery_item.payload, which is a genuine Map-typed jsonb column rather than one borrowed for the shape.
The STATUS tick is the only place a connector-reported state becomes Core state: pushed events merely ask for it, and what the connector answers here is what commits. The mapping follows the design's table, with the tail drain kept behind IN_PROGRESS so a run cannot enter PROCESSING while items are still at the connector. The attempt budget counts consecutive unanswered ticks rather than elapsed time. Any clear answer pulls the counter back to the ladder's ceiling, so a week-long scan and a days-long pause both keep their budget while the connector keeps answering; it runs out only when the connector goes silent. A run the connector no longer tracks ends immediately -- the contract makes that answer definitive. Two collaborators land with it: the v2 client, which rebuilds the identity and configuration every stateless-connector call replays, and the run terminator, which lands the status, the handle release and the agenda deletion together so a run can never end while still scheduled.
A drain tick pulls one bounded page, ingests it, and decides what comes next. When the connector says more is waiting, the worker commits the cursor advance and the agenda row's new due time and then publishes the follow-up tick itself, so the sweep's cadence is the recovery latency after a lost message rather than the drain's throughput ceiling. The handover to processing needs a full acknowledgement, not just an empty page: more:false on a completed run still allows the cursor to sit below the run-wide highestSequence, and swapping there would strand the items in between at a connector nobody will ask again. Anything short of a caught-up cursor drains again immediately; only a full ack swaps the agenda over, releases the connector handle and moves the run to PROCESSING. The attempt budget keeps counting consecutive failures only, so a page that answers -- items or a valid empty answer -- refreshes it. An empty answer refreshes without speeding the cadence up, since there is nothing to hurry toward.
Processing becomes a database cursor instead of one long message handler. Each tick claims a bounded batch of the run's unprocessed rows, runs the unchanged import pipeline over it, and either publishes the next batch's tick or ends the run. A pod that dies mid-batch simply leaves those rows unstamped, so any pod reclaims exactly them on the next tick -- closing v1's hole where a crash stranded a run in PROCESSING with no way back. The pipeline is shared, not reimplemented: the run-level decision is lifted out of importAndReport into importGroups, which both flows call. v1 keeps finishing a run in one pass through the finished event; a v2 run is ended by this worker, the only party that knows the backlog is empty. The final status is read from the evidence the rows carry, so a run with any recorded per-certificate reason ends WARNING rather than reporting a clean pass. Scope is certificates: keys are staged but have no import pipeline yet, so this worker neither claims them nor waits on them.
A PROCESS batch that falls short now adds its summary to the run's message log instead of leaving the run silent until it ends. An operator watching a long run can see what is going wrong while it is still going wrong, and the per-certificate reasons stay where they were, on the rows. The sentences move onto DiscoveryRunCounts so both flows say the same thing about the same counts: the v1 pass still turns them into its final status message, and a v2 batch files the identical wording per batch.
The reaper carried its own copy of the terminal mutation, and it had already drifted: unlike the terminator it never recorded the ending in the run's message log, so a run the reaper finished lost the one sentence explaining why. The mutation now lives once, on the terminator, and the reaper applies it to the run it already holds locked. Its own re-assert and row lock stay where they are -- those are the reaper's conditions, not the terminator's, and routing it through the transactional entry point would deadlock against the lock it is already holding.
The published schema for these fields promises curated text and no raw exception messages, and the producers added with the tick workers were concatenating getMessage() straight in -- putting transport internals, provider details and parser text where API clients read them. Certificate staging failures now go through the classifier that already exists for exactly this leak on processedError. Connector failures forward only an RFC 9457 detail, which the contract already obliges the connector to curate, and are classified otherwise. The full exception still reaches the log, which is where it was always the useful thing.
Four ways a v2 run could be killed, stranded, or double-imported. All were in the seams between the ticks rather than inside any one of them, which is why the suites were green. A run that has handed over to processing is alive but no longer holds a connector handle. Both connector-driven ticks tested only for a terminal run, so one still in flight from before the swap called the connector with no handle and read the resulting 404 as "this run no longer exists", ending a healthy run mid-import; a pushed event could re-arm that work for the same reason. They now ask whether the run has left the connector. The drain's drop was worse: it deleted the entire agenda, taking the PROCESS row that drives the remaining import, after which the reaper read a live run with no work as lost and failed it. A tick whose own work is obsolete now deletes only its own row. Processing looped instead of ending. The backlog was measured on `processed`, which the import pipeline deliberately leaves false on a row it never reached, so those rows were re-claimed every tick and the backlog never drained; the row was committed due-now and published, and a due-now row is one the sweep claims and publishes too, so two ticks ran the same batch and imported it twice; and nothing enforced the PROCESS budget, so none of it stopped. The claim now excludes rows already carrying a reason, the row is parked as a backstop rather than a competing publisher, and a tick that accounts for nothing climbs its ladder and ends the run. The not-tracked predicate missed the case its own javadoc claimed: over the proxy the problem body is discarded and the client raises a plain ConnectorEntityNotFoundException, so a forgotten run burned its whole budget. It also OR-ed code with status, letting an unrelated 404 read as not-tracked, and reached the status through HttpStatus.valueOf, which throws on a valid but unmapped code. It now mirrors the library's own predicate. The drain could also run forever: a connector counting more items than it handed over drove a due-now retry with the counter reset, and a page omitting the required `more` or `highestSequence` was read as "finished", handing a half-drained run to processing and releasing the handle. Both are now bounded by the budget. The handover sends the contract's full acknowledgement before releasing the handle, so a connector may discard the run's state rather than holding it for its full retention window. Ingestion is hardened alongside: a page for a run that ended while it was in flight is dropped, advisory events no longer write to a finished run, certificates are deduped within a page since their table has no uniqueRef constraint to lean on, a payload that does not match its declared resource costs itself rather than the page, and items arriving without the required sequence are recorded on the run instead of vanishing.
The two worst defects this round found lived in states no test set up: a STATUS or DRAIN tick arriving after the drain handed the run to processing, and a PROCESS batch that records reasons without stamping rows. Each new test fails against the code as it stood before the fixes. The drain's due-now assertions are rewritten as backstop assertions, since a row parked due-now is one the sweep claims and publishes itself.
The tick workers' class javadocs restated what their own methods already documented -- the backstop rule, the attempt budget, the continuation mechanism -- so the same fact lived in two places and only one of them would get corrected when the code moved. That already happened once on this branch: the drain's class doc still claimed the lagging-cursor branch retried immediately after the fix made it back off. Class docs now carry what spans the whole component; a method's mechanism stays on the method, where a reader stands when they are about to change it. The terminator's doc also loses a clause narrating a defect that no longer exists.
There was a problem hiding this comment.
Pull request overview
Implements the recoverable Discovery v2 ingestion and tick-driven lifecycle.
Changes:
- Adds sequence-gated ingestion and resource staging.
- Adds STATUS, DRAIN, and PROCESS workers with agenda-backed retries.
- Extends lifecycle bookkeeping, connector handling, and integration coverage.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/com/otilm/core/util/DiscoveryRunMetaFixture.java |
Adds run-handle fixtures. |
src/test/java/com/otilm/core/service/handler/discovery/DiscoveryEventIngestorTest.java |
Tests ingestion edge cases. |
src/test/java/com/otilm/core/messaging/jms/listeners/discovery/DiscoveryWorkListenerTest.java |
Tests tick dispatch. |
src/test/java/com/otilm/core/messaging/jms/listeners/discovery/DiscoveryRunReaperUnitTest.java |
Updates reaper lifecycle tests. |
src/test/java/com/otilm/core/integration/repository/DiscoveryRepositoryITest.java |
Tests typed run metadata persistence. |
src/test/java/com/otilm/core/integration/discovery/DiscoveryWorkSweepITest.java |
Updates sweep/reaping coverage. |
src/test/java/com/otilm/core/integration/discovery/DiscoveryStatusTickWorkerITest.java |
Tests STATUS transitions and retries. |
src/test/java/com/otilm/core/integration/discovery/DiscoveryProcessTickWorkerITest.java |
Tests processing batches and recovery. |
src/test/java/com/otilm/core/integration/discovery/DiscoveryEventIngestorITest.java |
Tests cursor and advisory ingestion. |
src/test/java/com/otilm/core/integration/discovery/DiscoveryDrainTickWorkerITest.java |
Tests draining and handover. |
src/test/java/com/otilm/core/integration/config/JsonColumnFormatMapperITest.java |
Moves JSON mapper verification to staged items. |
src/main/resources/application.yml |
Adds worker tuning properties. |
src/main/java/com/otilm/core/service/writer/DiscoveryWriter.java |
Adds run-message appending. |
src/main/java/com/otilm/core/service/writer/discovery/DiscoveryWorkWriter.java |
Adds work-type-specific deletion. |
src/main/java/com/otilm/core/service/writer/discovery/DiscoveryItemWriter.java |
Adds transactional item staging. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryV2Client.java |
Implements connector lifecycle calls. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryStatusTickWorker.java |
Implements STATUS ticks. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryRunTerminator.java |
Centralizes terminal transitions. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryProcessTickWorker.java |
Implements bounded processing ticks. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryEventIngestor.java |
Implements sequence-gated ingestion. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryDrainTickWorker.java |
Implements draining and processing handover. |
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryConnectorErrors.java |
Classifies connector failures. |
src/main/java/com/otilm/core/service/handler/CertificateHandler.java |
Adds transaction-joining certificate staging. |
src/main/java/com/otilm/core/model/discovery/DiscoveryRunLifecycle.java |
Adds shared lifecycle helpers. |
src/main/java/com/otilm/core/messaging/jms/listeners/discovery/DiscoveryWorkListener.java |
Dispatches agenda messages to workers. |
src/main/java/com/otilm/core/messaging/jms/listeners/discovery/DiscoveryRunReaper.java |
Reuses centralized termination logic. |
src/main/java/com/otilm/core/events/handlers/discovery/DiscoveryRunCounts.java |
Adds reusable warning descriptions. |
src/main/java/com/otilm/core/events/handlers/CertificateDiscoveredEventHandler.java |
Exposes bounded batch processing. |
src/main/java/com/otilm/core/dao/repository/DiscoveryWorkRepository.java |
Adds selective work deletion. |
src/main/java/com/otilm/core/dao/repository/DiscoveryItemRepository.java |
Adds idempotent native staging. |
src/main/java/com/otilm/core/dao/repository/DiscoveryCertificateRepository.java |
Adds processing cursor queries. |
src/main/java/com/otilm/core/dao/entity/Discovery.java |
Retypes connector run metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Six findings from the PR review, four of them residue from the previous round: a fix that covered a path but not its sibling. The full acknowledgement was sent before the cursor check, so a run whose cursor lagged told the connector it could discard the very items Core was still waiting for. It now goes out only once ingestion has caught up, and never on the lag path. Terminal status answers bypassed the under-lock ownership recheck, so a run the drain had safely handed over could still be failed or cancelled by a status answer in flight -- the same defect the last round fixed for live answers only. They now go through the same locked block, which also records the connector's own view of the ending rather than losing it. The handover re-asserted only that the run had not ended, so a concurrent pause or a second drain could still hand over and publish duplicate processing work. It now re-asserts the whole precondition. A status answer omitting the required run state reached getCode() inside the transaction and escaped the connector-call catch, retrying past its budget; it is now bounded like the drain's non-conformant page. A batch that failed wholesale was rethrown to the listener, which logs and acknowledges, so the budget was never spent and a persistent failure stranded the run in PROCESSING -- the failure this worker exists to close. It now takes the stall path. The terminal decision read only per-row errors, so a run whose bookkeeping write failed, or whose validation was never requested, reported a clean success. Run-level evidence counts too. The context-signature baseline rises to 61: three workers, three collaborator mock sets, three contexts.
The v1 entry point reaches the shared staging method by self-invocation, which Spring's proxy does not intercept, so the @transactional on it was inert -- and worse, it told a reader the method starts its own REQUIRED boundary when in practice it always runs in the caller's. Both callers already supply one: v1 through its own REQUIRES_NEW, v2 through the ingestor's. The native staging insert keeps its parameter-per-column signature, which is what binding a native query costs; suppressed with the reason rather than wrapped in an object that would be unpacked again at the only call site.
The v1 wrapper existed only to impose REQUIRES_NEW, and it reached the staging method by self-invocation -- which Spring's proxy does not intercept, so the boundary it was imposing never applied. Dropping the annotation from the inner method traded that for something worse: an un-annotated caller invoking a @transactional one, which cannot satisfy it at all. Both callers now reach the one annotated method through the proxy, and REQUIRED gives each what it needs. v1 calls from a virtual thread holding no transaction, so every batch still opens and commits its own and a later batch's failure cannot undo it. v2 calls from inside the ingestor's transaction and joins it, so staged rows and the cursor accounting for them still commit together.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 6 comments.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryStatusTickWorker.java:198
scheduleis an upsert that resets the DRAIN attempt to 0 and makes it due immediately. Because STATUS remains scheduled after aCOMPLETEDanswer, every later completed poll wipes out the counter thatawaitTheRestis spending; at the configured 5-minute STATUS cadence and 30-second DRAIN ceiling, the 100-attempt lag budget cannot be exhausted. Arm DRAIN only on the transition into completed state, schedule it only if absent, or retire STATUS when tail draining begins.
case COMPLETED -> {
run.setStatus(DiscoveryStatus.IN_PROGRESS);
clearResumeWindow(run);
workWriter.schedule(run.getUuid(), DiscoveryWorkType.DRAIN, OffsetDateTime.now(ZoneOffset.UTC));
src/main/java/com/otilm/core/dao/repository/DiscoveryCertificateRepository.java:43
- This ordered backlog query and its matching count execute for every 200-row batch, but the migrations define no
discovery_certificateindex coveringdiscovery_uuid, the outcome predicates, and creation order. Each tick therefore scans/sorts the run repeatedly (the count runs twice), making large-run processing quadratic. Add a suitable partial index and avoid full remaining-backlog counts where batch outcome accounting orexistsis sufficient.
@EntityGraph(attributePaths = {"certificateContent"})
List<DiscoveryCertificate> findByDiscoveryUuidAndNewlyDiscoveredTrueAndProcessedFalseAndProcessedErrorIsNullOrderByCreatedAsc(
UUID discoveryUuid, Pageable pageable);
long countByDiscoveryUuidAndNewlyDiscoveredTrueAndProcessedFalseAndProcessedErrorIsNull(UUID discoveryUuid);
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryDrainTickWorker.java:156
hasCaughtUpchecks only the cursor. A concurrent STATUS tick can move the run toSTOPPEDafter that check; this acknowledgement then lets the connector discard its state,swapToProcessingrejects the stopped run, and Core retains a resumable run whose handle no longer exists. Establish the handover while retaining the handle before acknowledging, or otherwise reserve/recheck connector ownership without holding a database lock across HTTP.
sendFullAck(run, page.getHighestSequence());
if (swapToProcessing(discoveryUuid, page.getHighestSequence())) {
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryProcessTickWorker.java:196
anyFailedcan be true solely because of a run-level message such as validation not being queued or bookkeeping failing, with no row carryingprocessedError. In that case the advertised certificate-list detail does not exist. Mention the run messages as the source for run-level warnings.
terminator
.end(discoveryUuid, DiscoveryStatus.WARNING,
"Discovery completed with warnings. See the discovery certificate list for "
+ "per-certificate detail.");
Six findings from the second review round, four of them the same shape as the last: a request that went out before the handover, answering after it. A status or drain call started while the run was still connector-owned can return 404 once the handover has released the handle. Every failure path took that as "the run no longer exists" and ended a healthy import, deleting the agenda row driving it. The terminator gains a connector-owned entry point that refuses a run which has left the connector, and every connector-driven ending now goes through it; processing keeps the plain one, since ending a PROCESSING run is exactly its job. The handover's own PROCESS row was scheduled due-now and then published directly -- the double-publisher bug fixed for continuations and missed at the handover itself. It is parked at the backstop like the rest. Ingestion sat outside the connector-call catch, so a page that could not be staged escaped to the listener and was redelivered forever without the budget advancing. It now spends budget like any other unanswered tick. Connector-supplied prose no longer reaches the run's message. The contract obliges a connector to curate its RFC 9457 detail, but an obligation is not a guarantee and the run message is user-visible; only the error code survives, mapped to text Core wrote. The connector's wording still reaches the log. The stall-limit message pointed operators at a certificate list that has nothing to show for those rows, since they carry no per-row reason; it now points at the run's messages, where the batch failure is recorded. Tests cover the classifier's arms, what the v2 client puts on the wire including the maxBytes clamp and the acknowledgement cursor, and the drain paths these fixes added.
Four defects a third review round found, three of them ways a run either loses itself or never ends. The full acknowledgement went out before the handover committed. It lets the connector discard the run's whole state, so a swap that then rolled back or lost a race left the connector free to throw away a run Core never finished taking over -- and the next drain would meet a 404 and fail a run whose items were all safely staged. It now goes out after the swap commits, replayed against the handle that swap released. Every repeated `completed` status answer re-armed the drain row, and arming resets the attempt counter, so a permanently failing drain had its budget refreshed faster than it could spend it and the run never ended. The row is armed on the transition only. Processing claimed rows by page while the import pipeline acts once per certificate content, so a certificate found on more hosts than the batch size had its group split across ticks and its triggers, histories and validation run once per page. The claim pages by content instead, and takes every pending row of the contents it claims. A `more: true` page carrying nothing new was answered by publishing another tick at once, which spins as fast as the connector repeats itself. Ingestion now reports whether the cursor moved, and a page that did not move it takes the bounded path. Also drops a boilerplate javadoc sentence, shortens the groundwork note on the advisory path, and rewrites the acknowledgement's own doc to match its new ordering.
lubomirw
changed the title
discovery_certificate has had no index since it was created -- the foreign key to discovery is a constraint, and PostgreSQL does not index the referencing side -- so the claim, its two backlog counts and the end-of-run warning check have all been sequential scans, once per tick. Both indexes are partial. A row leaves the pending set permanently once it carries an outcome, so that index shrinks as a run drains rather than growing with it, and the failed-row index holds nothing at all on a healthy run. The claim also breaks ties on the content id now. i_cre comes from the JVM clock and the staging loop writes rows faster than it advances, so two groups stamped in the same tick could otherwise swap places between pages -- which made the javadoc's promise of a stable order untrue.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryDrainTickWorker.java:137
- This terminal message is inaccurate when
moreis present buthighestSequenceis missing: the connector did say whether items remain. Name both required pagination fields so operators can diagnose either malformed response.
"The connector's results did not say whether more items remain");
src/main/java/com/otilm/core/service/handler/discovery/DiscoveryProcessTickWorker.java:74
- Validate this tunable before storing it. With
discovery.processing.batch-size=0(or a negative value),PageRequest.of(0, batchSize)throws before the worker reaches its bounded stall path; the listener acknowledges each failure and the agenda retries forever, leaving the run inPROCESSING. Failing application startup makes the misconfiguration actionable.
this.batchSize = batchSize;
Arming an agenda row is a fresh start and resets its attempt counter, and every caller meaning only "look now" inherited that. A pushed event is not an answer from the connector, so a connector whose endpoint was broken but whose event stream worked could keep refreshing the failure budget and hold a dead run open indefinitely. The agenda gains an expedite operation: same upsert, without the counter reset in its conflict branch. New rows still start at zero; existing ones keep what they have spent. Arming stays for the genuine fresh starts, the drain handover and the first completed status answer. This is the second instance of the same cause. A previous round fixed the completed-status path re-arming the drain row on every poll, but fixed the instance rather than the reset itself. Certificate metadata definitions are also registered once per drained page before its rows can reach the pipeline. The pipeline imports content groups in parallel and each applies its rows' metadata, so two groups carrying the same new definition raced to insert it and the loser's whole group rolled back as failed -- a certificate reported unimportable for no reason but timing. The v1 download path has always done this before submitting a batch; v2 never did.
An independent review of the whole branch found the same accounting duplicated in the status and drain workers, down to a verbatim comment. Both copies now delegate to DiscoveryTickBudget, so a fix to how an unanswered tick is charged can no longer land in one worker and not the other. Draining a page Core cannot store is charged to the same budget but ends the run with Core named as the party that failed. The old path routed it through the connector's message, which told an operator the connector had stopped handing over items when it had answered correctly. Also drops two pieces of sediment the review turned up. The drain's hasCaughtUp pre-check re-read the cursor in its own transaction to decide what swapToProcessing re-asserts under the run row's lock, and the caller answered both the same way. The status worker set connector_status twice per live answer. The remaining comment corrections are all cases where the code moved and the prose did not: applyTerminalState has two lock-holding callers rather than the one its javadoc named, applyDrainPage's idempotency paragraphs sat inside its @return tag, the ingestor's terminal-only guard never said why PROCESSING is deliberately still open to staging, and claimContents claims nothing until core#2130 lands.
The full ack is the one call that licences a connector to discard a run's state, and no test asserted it was sent, that it carried the run's highest sequence, or that it was withheld while the cursor still lagged. It now has all three, plus the case that matters most: an ack that fails leaves the handover standing, because the connector retains the run for 24 hours regardless and rolling back would cost the import instead. Three budget-exhaustion endings were also unpinned -- processing that never accounts for its backlog, a connector that keeps omitting the run state, and a page Core keeps failing to store. Each is what stops a run sitting in its phase forever, so each is worth a test. Renames ingestionFailure_spendsBudgetRatherThanEscapingToTheListener, which asserted the attempt counter was zero. The budget is spent by the sweep's claimer, which this test bypasses; what it actually pins is that the failure is contained, the cursor is untouched and the agenda row survives. It now says so and asserts all three.
The process worker read its backlog outside any lock, so a drain page still in flight across the handover could stage rows after the count and before the ending committed -- staged, counted by nobody, never imported. The zero-backlog check now runs inside the terminal transaction, where it serialises against staging: both take the run row's lock, so a late page either commits in time to be imported or finds the run already terminal and is refused. When the check finds work after all, the run carries on processing instead of ending. This is what makes the ingestor's terminal-only guard defensible. Its comment previously claimed refusing a late page was the only way those items could vanish; they could vanish through this race just as silently, so the comment now describes the lock ordering that actually holds. Also splits the two kinds of terminal evidence. A run warned on run-level evidence alone has no row carrying a reason, so pointing the operator at the discovery certificate list sent them to a list of clean rows. Found by the Copilot review on #2129.
A hygiene pass over the branch. The substantive one: both tick workers carried a verbatim comment claiming DiscoveryV2Client's javadoc warns its declared throws is incomplete. It carries no such warning, and the concrete cases the comment lists -- a 422 arriving as an unchecked ValidationException, a bodiless 2xx as IllegalStateException -- make the point without the citation. The rest is length. Four javadocs restated their own summaries or the repository javadoc next door, and the tick budget's class comment narrated how the duplication came about rather than why one copy matters.
Moving the backlog check into the terminal transaction last round fixed one of three inputs and left the other two outside it. A late drain page takes the same lock and can both stage rows and append to the run's message log, so a page carrying only malformed items -- or only keys, or certificates that all failed to stage -- appends a complaint, leaves the certificate backlog at zero, and the ending commits the stale decision: a run reported as completed successfully while carrying a warning. endWhile is replaced by endWith, which hands the locked entity to a callback and applies whatever ending it returns, or none. The process worker now reads the backlog, the per-row failures and the message log there, and reads the log off the locked entity rather than issuing its own query, so it cannot be stale by construction. Two smaller ones alongside. The results page's items field is required on the wire -- a page with nothing to hand over sends an empty array -- so a missing items now joins more and highestSequence in the non-conformant guard, rather than being normalised to an empty page that reads as fully drained and releases the connector handle. And the tick workers reject non-positive bounds at construction: a batch size of zero throws inside PageRequest.of before the worker reaches any bounded path, where the listener acknowledges it and nothing ever ends the run. Found by the Copilot review on #2129.
The wire contract defines uniqueRef as the key Core dedupes an item by "across drains and retries". discovery_item has honoured that since the v2 schema, through uq_discovery_item_ref. discovery_certificate predates v2 and had nowhere to put the reference, so v2 certificate dedupe was per-page and in memory: a connector re-sending an item under a newer sequence -- which the contract permits, and which the cursor filter lets through by design -- staged it a second time and the run imported the same certificate twice. The column is nullable and the unique index partial, because v1 has no equivalent key. A v1 provider's certificate uuid names the certificate rather than the occurrence, and nothing in the v1 contract promises it appears once per run, so writing it here would impose a guarantee no provider has been asked to meet. Both staging callers now say which they are rather than the shared path guessing. The ingestor looks up only the references its own page carries, not the run's whole set, which on a large run is every certificate it has found. Reading and filtering is enough because every drain for a run serialises on the row lock the ingestor already holds; the index is the backstop. The index migration this branch added is extended rather than followed by a second one -- it is unreleased, so no deployed database has run it. Anyone who has already applied it on this branch needs a flyway repair or a fresh database. Found by the Copilot review on #2129.
importGroups reports a percentage of the groups it was handed, and its last group always makes that 100%. For v1 that is right -- one pass imports the whole run. A v2 processing tick is handed one bounded batch, so every batch was writing "Processed 100 % of newly discovered certificates" while the run still had a backlog. totalGroups becomes nullable and means what its name says: the run's total, or absent when the caller sees only part of the run and reports its own progress. The PROCESS worker then writes the message itself, as a remaining count rather than a fraction -- it is the only party that knows what is left, and the backlog is what a tick already measures, so this costs no query. Deriving a run-wide denominator instead would scan discovery_certificate on every tick, since processed rows fall outside both of the table's partial indexes. Also validates continuation-backstop in both tick worker constructors, which the previous round missed while adding the other three. Zero or negative parks the backstop row due-now while the worker publishes the continuation itself, so the sweep claims it and publishes a competing tick -- the race the backstop exists to prevent, and it would read as duplicate processing rather than as a misconfiguration. Found by the Copilot review on #2129.
| if (apply(discoveryUuid, status)) { | ||
| // A clear answer refreshes the budget without restarting the backoff ramp: the counter drops to | ||
| // the rung where the ladder already reached its slowest delay. | ||
| workWriter |
There was a problem hiding this comment.
! Major | Correctness
DiscoveryStatusTickWorker.java:101 resets the persisted attempt while DiscoveryWorkClaimer can concurrently read the old value and later overwrite the reset. DiscoveryProcessTickWorker.java:212 and DiscoveryDrainTickWorker.java:234 also trust attempts carried by delayed messages, so an old high attempt can terminate after a newer success, while an old low attempt can reduce the persisted counter. This violates this PR's consecutive-unanswered-ticks budget and can prematurely fail a healthy run or retry indefinitely.
Suggested: Serialize agenda claim, reset, and spend operations with a row lock or generation-based compare-and-set, ignoring stale tick attempts before terminating or rescheduling.
flagged by Transaction & concurrency
There was a problem hiding this comment.
Valid, and folded into #2130 rather than answered here. Both halves hold: resetAttempt does not take the sweep's advisory lock, so a claimer that has already read the old value overwrites the reset; and the workers trust the attempt their message carries, so a stale redelivery either ends a recovered run or rewinds the budget.
It went to #2130 rather than a new issue because it is the same root shape as the batch claim already tracked there -- agenda state mutated from two places with no shared lock -- and it needs the same concurrent-ticks harness that issue's DoD already calls for. #2130 is now titled "Serialise discovery agenda claims and processing batches" and carries two DoD items for this.
| if (batch.isEmpty()) { | ||
| return new DiscoveryRunAccumulator().counts(); | ||
| } | ||
| EventMessage eventMessage = constructEventMessage(discovery.getUuid(), null, null); |
There was a problem hiding this comment.
! Major | Correctness
CertificateDiscoveredEventHandler.java:286 creates the PROCESS event with a null user, while DiscoveryWorkListener.java:42 invokes this path without installing an Authentication. AuthorizationEnforcerImpl.java:44-48 therefore denies the check at CertificateDiscoveredEventHandler.java:295, causing every non-empty batch to stall and eventually end as WARNING without importing certificates.
Suggested: Carry and authenticate the initiating user UUID for PROCESS ticks, or install a defined system principal authorized for CERTIFICATE:CREATE before invoking processBatch.
flagged by Security & info-leak
There was a problem hiding this comment.
Confirmed, and this was a blocker. I traced it: EventListener.processMessage calls authenticateAsActingUser when the event carries a userUuid, which is how v1 gets an identity on a JMS thread. DiscoveryWorkListener installed nothing, so enforce read a null Authentication and refused -- and because importBatch swallows to reach its bounded stall path, the refusal would have surfaced as a run that quietly imported nothing and ended WARNING.
Took your first suggestion. The run now records who started it (started_by_user_uuid, captured at creation while a caller is still on the thread) and the worker authenticates as them before each batch.
The proof was uncomfortable: setting the fixture's user turned 9 of 20 process tests red immediately. They passed before only because the ITests mock the import pipeline, so enforcement was never exercised. There is now a test asserting authenticateAsUser is called with the run's user. a5b5d8e
| * Deliberately bare for the schema groundwork: the staging writes, the union listing and the processing claims each | ||
| * bring their own queries with the tasks that own them. | ||
| * Staging store for every discovered resource except certificates, which keep their own v1 table until the | ||
| * evidence-gated unification (core#2027). |
There was a problem hiding this comment.
. Nitpick | Documentation
This PR adds core#... breadcrumbs at DiscoveryItemRepository.java:14, DiscoveryEventIngestor.java:260, DiscoveryProcessTickWorker.java:42,134, and DiscoveryProcessTickWorkerITest.java:157. The surrounding prose already explains the relevant invariants, while the PR description is the durable home for these tracking links.
Suggested: Remove the five core#... references while retaining the adjacent behavioral rationale.
flagged by Noise & hygiene
There was a problem hiding this comment.
Agreed, all five removed with the behavioural statements kept. 06abf7e
| || key.getFingerprint().isBlank()) { | ||
| return true; | ||
| } | ||
| return keyItemRepository.findByFingerprint(key.getFingerprint()).isEmpty(); |
There was a problem hiding this comment.
**~ Minor** | Performance
DiscoveryEventIngestor.java:241-244 loops over every drained item, while DiscoveryEventIngestor.java:324 calls findByFingerprint for each key. A 500-key page therefore adds 500 SELECTs while applyDrainPage holds the run's pessimistic lock.
Suggested: Collect key fingerprints before the loop, fetch existing fingerprints once with an IN projection, and compute newlyDiscovered from that set.
flagged by Persistence & schema
There was a problem hiding this comment.
Valid. Fixed: the page's key fingerprints are collected once and resolved with a single IN query, and novelty is computed from that set. It mattered more than the round-trip count alone suggests, since this runs inside the transaction holding the run's row lock -- everything else about the run queues behind it. a5b5d8e
|
|
||
| /** Every pending row of the given contents, so a claimed group is always whole. */ | ||
| @EntityGraph(attributePaths = {"certificateContent"}) | ||
| List<DiscoveryCertificate> findByDiscoveryUuidAndCertificateContentIdInAndNewlyDiscoveredTrueAndProcessedFalseAndProcessedErrorIsNull( |
There was a problem hiding this comment.
**~ Minor** | Performance
DiscoveryCertificateRepository.java:57 returns every pending row for the selected content IDs without pagination. DiscoveryProcessTickWorker.java:148-155 deliberately accepts the first group even when its weight exceeds batchSize, so one certificate reported at arbitrarily many locations bypasses the configured batch limit and materializes the entire group in memory.
Suggested: Introduce an explicit maximum group size with bulk handling for oversized groups, or redesign the claim so rows can be paged without repeating group-level side effects.
flagged by Persistence & schema
There was a problem hiding this comment.
Correct, and deliberate today -- the group wins because splitting it would run its triggers, histories and validation once per page. But you are right that it is unbounded, and "documented trade-off" is not the same as "safe".
Tracked on #2130 rather than fixed here: that issue now covers the claim redesign this needs, and an explicit maximum group size only makes sense alongside the bulk handling for oversized groups you describe.
| * Consumes {@code provider.discovery-work} ticks and hands each to the worker for its type. | ||
| * | ||
| * <p> | ||
| * <b>No {@code @Transactional} here:</b> a tick calls the connector, and a connector call must never run inside a |
There was a problem hiding this comment.
**~ Minor** | Documentation — comments state what the code is not
Comments should say what a thing is, not what it isn't. 64 added comment lines are framed as a negation or a contrast — a reader has to hold the rejected alternative in mind to extract the actual statement, and the rejected alternative is not in the code.
This line is the pattern at its clearest: <b>No {@code @Transactional} here:</b> documents an annotation that isn't present. Others:
DiscoveryEventIngestor.java:292— "Names the page by what it carried, not by where the cursor stood before it."DiscoveryEventIngestor.java:277— "the table's partial unique index is the backstop, not the mechanism."DiscoveryDrainTickWorker.java:280— "Parked, not due-now"DiscoveryEventIngestor.java:336— "Expedited, not armed"DiscoveryItemWriter.java:36— "The enum's name, not its wire code"DiscoveryV2Client.java:104— "Clamped rather than trusted"DiscoveryWorkWriter.java:50— "{@code REQUIRED} (not {@code REQUIRES_NEW})"DiscoveryProcessTickWorker.java:81— "A backstop that is not in the future is not a backstop."CertificateDiscoveredEventHandler.java:269— "Deliberately says nothing about the run as a whole: it emits noDISCOVERY_FINISHED, decides no final status, and does not care whether more batches follow."DiscoveryConnectorErrors.java:24— "Nothing the connector wrote is forwarded — not {@code getMessage()}, and not the RFC 9457 {@code detail} either."- Three ITest class docs open with "Not {@code @transactional}: …"
Suggested: Restate each positively. No @Transactional here: a tick calls the connector… → Each worker opens its own short transactions; a connector call runs outside any transaction. Parked, not due-now → Parked one backstop interval out. Clamped rather than trusted → Clamped to the contract's cap.
flagged by pr-hygiene · obvious-comment
There was a problem hiding this comment.
Right as a rule, and applied -- b0be9c5 restated the sites you listed positively, including the three ITest class docs.
I kept two as negations, and want to flag them rather than have it look like an oversight. Parked, not due-now and REQUIRED (not REQUIRES_NEW) exist because the negated alternative is the tempting wrong edit, and both were actual bugs on this branch -- a due-now row lets the sweep publish a competing tick, and REQUIRES_NEW stops reschedule joining the claimer's transaction. Restating them as facts keeps the fact and loses the warning. They are reworded but still name what must not happen.
| if (batchSize <= 0) { | ||
| throw new IllegalArgumentException("discovery.processing.batch-size must be positive"); | ||
| } | ||
| // A backstop that is not in the future is not a backstop. The row would be parked due-now while this |
There was a problem hiding this comment.
**~ Minor** | Documentation — the same rationale is restated at many sites
Several explanations are repeated near-verbatim across the diff, so each one now has many copies to keep true.
- The backstop / due-now race is explained at six sites: here,
DiscoveryDrainTickWorker.java:79-81,:280-281,:295-297,DiscoveryProcessTickWorker.java:188-190, andDiscoveryTickWorkerConfigTest.java:39-41. - "strands the run in PROCESSING forever" appears at 8 sites.
- The listener's log-and-acknowledge consequence is spelled out at 7 sites, twice word-for-word: "RuntimeException too: over MQ a 422 arrives as an unchecked ValidationException and a bodiless 2xx as IllegalStateException…" is identical in
DiscoveryDrainTickWorker.java:109-111andDiscoveryStatusTickWorker.java:76-78, as is "Outside any transaction, by the platform's connector-call rule." at:106and:73. - The tunneled-404 and REGISTRATION_NOT_FOUND-on-422 reasoning is given in full in both
DiscoveryConnectorErrors.java:51-63and its test atDiscoveryConnectorErrorsTest.java:31-40.
Suggested: Keep one authoritative copy — on DiscoveryTickBudget for the budget rules, on DiscoveryWorkWriter.schedule/expedite for the backstop rules, on DiscoveryConnectorErrors for the 404 rules — and let the other sites carry a bare {@link} or nothing at all.
flagged by pr-hygiene · doc-duplication
There was a problem hiding this comment.
Agreed, and done exactly as you suggest -- one authoritative copy each, the rest pointing at it. The backstop rules live on DiscoveryWorkWriter, the stranded-in-PROCESSING failure on DiscoveryProcessTickWorker's class doc, the log-and-acknowledge consequence on DiscoveryWorkListener, and the tunneled-404 reasoning on DiscoveryConnectorErrors with its test shortened to a pointer. b0be9c5
| } | ||
| Discovery run = located.get(); | ||
| if (DiscoveryRunLifecycle.isTerminal(run.getStatus())) { | ||
| // Terminal only, where every other guard in the engine uses hasLeftTheConnector. PROCESSING is |
There was a problem hiding this comment.
**~ Minor** | Documentation — inline comment volume
881 of this PR's 4478 added lines are comments — around 20%, and the inline blocks are paragraphs rather than notes. This one runs five lines; DiscoveryDrainTickWorker.java:122-125 and :134-137 run four each, as do DiscoveryProcessTickWorker.java:173-177 and :257-260, and DiscoveryStatusTickWorker.java:91-93, :145-147, :151-153, :186-187, :191-193.
Inline comments should be sparing — a line or two where the code genuinely cannot say it. At this density the comments become the primary text and the code the annotation, and every one of them is a separate thing that can drift out of date.
Suggested: Cut each inline block to the single non-obvious sentence, and move the argued-for-the-reader parts (why the alternative was rejected, what the v1 flow used to do, what would go wrong in a design that isn't in the tree) to the PR description. For this block, the load-bearing sentence is the last one — that the run row lock is what makes staging into a PROCESSING run safe.
flagged by pr-hygiene · obvious-comment
There was a problem hiding this comment.
Fair, and the number was the useful part -- it is not something I would have noticed from any single block. Two passes took the diff down by roughly 290 lines of prose: b0be9c5 for the duplication and the negation framing, 06abf7e for the entities, repositories, migration, and the ingestor, process worker and terminator.
For this block specifically you identified the load-bearing sentence correctly, and that is what survives: the run row lock is what makes staging into a PROCESSING run safe.
| @Autowired | ||
| private DiscoveryWorkWriter workWriter; | ||
|
|
||
| // ------------------------------------------------------------------ continuation |
There was a problem hiding this comment.
. Nitpick | Documentation — ASCII section banners in tests
20 banner comments of the form // ------------------------------------------------------------------ continuation divide the added ITests. Four of them read fixtures, three ticks with nothing to do, two unanswered ticks.
They restate grouping that the test method names already carry, and they are the kind of decoration that silently stops matching once a test is added under the wrong banner. JUnit has @Nested for this, which the compiler keeps honest.
Suggested: Replace the banners with @Nested classes, or drop them — the method names are the labels.
flagged by pr-hygiene · obvious-comment
There was a problem hiding this comment.
Agreed, and removed from all five test files. b0be9c5
I did not convert them to @Nested: that changes test structure rather than comments, and this was a documentation pass. Worth doing separately if you want the compiler enforcing the grouping -- the method names carry it for now.
| workRepository.schedule(UUID.randomUUID(), discoveryUuid, workType.name(), nextDueAt); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
. Nitpick | Documentation — writer Javadoc restates the repository's
DiscoveryWorkWriter.expedite documents what DiscoveryWorkRepository.expedite already documents at length (DiscoveryWorkRepository.java:46-56, 11 lines), and the same pair repeats for deleteForRunAndType: the writer's 9-line block at DiscoveryWorkWriter.java:75-83 and the repository's at DiscoveryWorkRepository.java:89-92 explain the same distinction. The writer is a one-line delegation in both cases.
When a fact lives in two doc comments, one of them rots. Also note DiscoveryRunTerminator.java:56, :63 and :77 carry the identical bare tag @return whether this call was the one that ended the run three times.
Suggested: Let the repository own the rules and give the writer a {@link DiscoveryWorkRepository#expedite} reference, or drop the writer's doc entirely — the delegation is self-evident.
flagged by pr-hygiene · doc-duplication
There was a problem hiding this comment.
Agreed. The repository owns the agenda rules now and the writer's javadoc is one line per method; the schedule/expedite distinction and the deleteForRunAndType reasoning are stated once. The three identical @return tags on DiscoveryRunTerminator went with the rest of that class's javadoc in 06abf7e.
|
In general, this PR's comments argue the design — what the v1 flow did wrong, which alternative was rejected, what would break in a version that isn't in the tree. That's PR-description material, not a part of the source code. |
A PROCESS tick arrives on a JMS thread with no principal, and the import pipeline enforces CERTIFICATE:CREATE against whatever is on the thread. Nothing installed one, so authorization refused every batch -- and because importBatch swallows to reach its bounded stall path, the refusal surfaced as a run that quietly imported nothing and ended WARNING. The ITests missed it because they mock the pipeline, so the enforcement was never exercised. The v1 flow solves this by carrying the user on its CERTIFICATE_DISCOVERED event, which its listener authenticates from. A tick has no such envelope, so the run holds the user instead: captured at creation while a caller is still on the thread, and installed before each batch. Three more from the same review: The swap to PROCESSING returned a nullable handle, using null for both "no handover" and "a handover whose connector handle was absent" -- and meta is optional in the contract, so a legitimately handle-less run was read as no swap and skipped both the PROCESS publication and the full acknowledgement. It now returns a Handover. discovery_certificate's unique constraint lived only in the migration. Tests build their schema from the entities, so the dedupe invariant added for uniqueRef was one the tests could not exercise; it is now mapped as well as migrated. Key novelty asked inventory one fingerprint at a time, inside the transaction holding the run's row lock -- 500 round trips for a full page, with every other write about the run queued behind them. One query per page now.
Roughly a fifth of this PR's added lines were comments, and the prose had started to argue rather than explain: rationale repeated at up to eight sites, paragraphs describing what the code deliberately is not doing, and forward references to issues that will change it later. Four rationales now have one authoritative home each -- the backstop race on DiscoveryWorkWriter, the stranded-in-PROCESSING failure on DiscoveryProcessTickWorker, the listener's log-and-acknowledge on DiscoveryWorkListener, the tunneled 404 on DiscoveryConnectorErrors -- and their other sites point at it instead of restating it. Negation framing is restated positively, five core#NNNN breadcrumbs are gone with their behavioural statements kept, the longest javadocs are cut to a summary plus tags, and the ASCII section banners are gone from the ITests, where the method names were already the labels. Two guard comments were left as warnings rather than facts, because each describes an edit that was an actual bug on this branch: the backstop row must be parked ahead of due-now, and reschedule must be REQUIRED so it joins the claimer's transaction. Clears both open Sonar issues, which had the same cause -- a member inserted between a doc or annotation and the declaration it belonged to. progress and runMeta each state their own S1948 justification now, with the condition that ends it, and the Handover record no longer sits between handleUnanswered's javadoc and the method.
A second pass over the surfaces the first one spared. The migration was the worst of them: forty-two lines of essay above five DDL statements, arguing the design and comparing it to v1. It is now nineteen -- each statement saying what it does plus the one thing the DDL cannot show, why a column is nullable or an index partial. Entity field comments say what the column holds and stop. Repository query javadoc is one sentence, with a second only where a predicate genuinely traps a reader: the claim query's "accounted for, not processed" rule survives, the paging-by-content reasoning and the schedule/expedite concurrency essay do not -- DiscoveryWorkWriter owns the agenda rules now. The ingestor, the process worker and the terminator lose their class-level narration and their method javadoc drops to a sentence each. Four explanations were kept because nothing else carries them: the cursor advances by what a page held rather than the run-wide count, the run row lock is what makes staging into a PROCESSING run safe, importBatch swallows so a failure reaches the bounded stall path, and applyTerminalState exists for callers already holding the lock. 79 insertions against 285 deletions.
|
TLDR
Implements the discovery v2 engine in Core: a sequence-gated ingestor and the three tick workers (
STATUS,DRAIN,PROCESS) that drive a run off thediscovery_workagenda. Every tick is short, stateless and idempotent, so a pod dying mid-run is recoverable by any other pod — closing v1's hole where a crash stranded a run inPROCESSINGwith no way back.Closes #1963, under the discovery v2 epic OmniTrustILM/ilm#267.
What lands
Ingestor — one funnel for connector-reported data. A drain page stages its items under the run row's lock and advances
last_applied_sequenceby the highest item sequence the page actually carried, never by the response's run-widehighestSequence, which counts items the connector has produced but not handed over. Advancing by the latter silently skips everything in between; it has its own regression test. Certificates keep the v1 staging table (a REQUIRED variant of that write, so staged rows and the cursor accounting for them commit together); every other resource lands indiscovery_item, deduped on the connector'suniqueRef.STATUSworker — the only place a connector-reported state becomes Core state. The connector-state mapping is applied here, with the tail drain kept behindIN_PROGRESSso a run cannot enterPROCESSINGwhile items are still at the connector. The attempt budget counts consecutive unanswered ticks, so a week-long scan and a days-long pause both keep their budget while the connector keeps answering.DRAINworker — one bounded page, then a decision. Continuations are direct-published with the agenda row parked as a backstop. The handover toPROCESSINGrequires a full acknowledgement, not just an empty page, and sends the contract's ack drain before releasing the connector handle.PROCESSworker — processing becomes a database cursor instead of one long message handler. Bounded batches through the unchanged import pipeline; a batch that dies leaves its rows reclaimable. TerminalWARNINGorCOMPLETEDis read from the evidence the rows carry.Also:
discovery.run_metais retyped toList<MetadataAttribute>. It holds the connector's run handle, which the wire carries as an attribute list, but the field wasMap<String, Object>— unable to represent a JSON array at all — so the first code to replay the handle could not read one back.Review round already applied
A multi-reviewer pass (guided + superpowers + hygiene; Copilot stalled and is recorded as a failed source) found 28 findings, and every actionable one is fixed in this branch. The three worst were all in the seams between ticks, which is why the suites were green:
DRAINtick on aPROCESSINGrun deleted the whole agenda, taking thePROCESSrow driving the import — after which the reaper read a live run as work-lost and failed it.STATUShad noPROCESSINGguard, so a tick from before the handover called a handle-less connector and read the 404 as "this run vanished", ending a healthy run mid-import.PROCESSmeasured its backlog onprocessed, which the pipeline deliberately leaves false on rows it never reached — so those were re-claimed every tick, the row was committed due-now and published (making the sweep a competing publisher, hence double imports), and no budget ever stopped it.Plus: the not-tracked predicate missed the tunneled 404 its own Javadoc claimed to cover; unchecked connector failures bypassed the budget entirely; the cursor-lag re-drain was a backoff-free retry storm; and a page omitting the required
more/highestSequencewas read as "drain finished".Testing
71 discovery tests (12 ingestor ITests + 5 unit, 14
STATUS, 9DRAIN, 12PROCESS, 4 listener, plus the existing sweep and repository suites), each new race test failing against the pre-fix code.DiscoveryServiceITest(21) and the certificate-handler suites unchanged, so v1 behaviour stays frozen.TransactionalBoundaryArchTestandConnectorApiClientArchTestgreen.Deliberately not here
Stopped/Cancelledand the absentdiscovery-provider-v2.mdbelong to Discovery v2 provider documentation and quick start documentation#355, the v2 API-reference group to Discovery v2 interface documentation and OpenAPI sync interface-documentation#219.PROCESSINGswap, so it is sent at the last moment the handle exists. Reconciling the two belongs to Discovery v2 adapter lifecycle and core API #1964.