Improve parallel writes - #353
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request improves parallel write reliability and fairness around DroneDB index updates by introducing a per-dataset coalescing queue, adding typed handling for transient DB contention, and adding a recurring reconciliation sweep to catch/repair unindexed-on-disk files. It also refactors the DroneDB dataset API into role-based interfaces to reduce coupling for narrow consumers.
Changes:
- Add
DdbBusyException+DdbResult.Busyand propagate/handle transient contention consistently (including Hangfire retry policies). - Introduce
IDatasetIndexQueue+DatasetIndexQueueto coalesce concurrent index writes into native batch calls with per-item isolation, plus reconciliation/quarantine flows. - Split
IDDBinto role interfaces (IDdbIndex,IDdbBuild,IDdbMeta,IDdbRaster,IDdbAnalytics) and add batch-add result types for the new native completeness contract.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Registry.Web/Utilities/ProcessingNodeServiceCollectionExtensions.cs | Registers IDatasetIndexQueue and IndexReconciliationService for processing nodes. |
| Registry.Web/Utilities/HangfireUtils.cs | Expands Hangfire retries with backoff for transient DDB contention/build-lock scenarios. |
| Registry.Web/Utilities/ApiExceptionFilter.cs | Adds global exception-to-HTTP mapping, including transient 503 + Retry-After handling. |
| Registry.Web/Startup.cs | Registers ApiExceptionFilter and wires IDatasetIndexQueue/IndexReconciliationService in DI. |
| Registry.Web/Services/Ports/IDatasetIndexQueue.cs | Defines the per-dataset coalescing index-write queue contract. |
| Registry.Web/Services/Managers/ObjectsManager.cs | Routes indexing through IDatasetIndexQueue when available and quarantines files on indexing failures. |
| Registry.Web/Services/Initialization/HangfireJobsInitializer.cs | Schedules the recurring “index-reconciliation” Hangfire job. |
| Registry.Web/Services/HeavyTasks/Adapters/HeavyTaskJobWrapper.cs | Applies transient-only Hangfire backoff retry policy to heavy tasks. |
| Registry.Web/Services/Adapters/IndexReconciliationService.cs | Adds recurring sweep to re-enqueue unindexed files, report missing files, and age out quarantine. |
| Registry.Web/Services/Adapters/DatasetIndexQueue.cs | Implements per-dataset FIFO lanes with batching, retries, and per-item completion isolation. |
| Registry.Web/Models/Configuration/ReconciliationSettings.cs | Adds config tuning for reconciliation sweep behavior. |
| Registry.Web/Models/Configuration/IndexQueueSettings.cs | Adds config tuning for batching/coalescing behavior and deadlines. |
| Registry.Web/Models/Configuration/AppSettings.cs | Adds app settings hooks for index queue + reconciliation configuration and cron. |
| Registry.Web/Exceptions/TransientException.cs | Adds a generic retryable exception mapped to 503 + Retry-After. |
| Registry.Web/appsettings-default.json | Provides defaults for index queue/reconciliation settings and cron schedule. |
| Registry.Web.Test/ProcessingNodeDiCompletenessTests.cs | Verifies DI completeness for the new queue and reconciliation service on processing nodes. |
| Registry.Web.Test/ObjectManagerTest.cs | Adds coverage for quarantining behavior when indexing fails after file placement. |
| Registry.Web.Test/IndexReconciliationServiceTests.cs | Adds unit tests for reconciliation re-enqueue, reserved path exclusion, report-only behavior, and quarantine aging. |
| Registry.Web.Test/DatasetIndexQueueTests.cs | Adds unit tests for batching/coalescing, per-item errors, unchanged resolution, retry behavior, and completeness gaps. |
| Registry.Ports/IDdbWrapper.cs | Adds native wrapper surface for “add with options” returning completeness results. |
| Registry.Ports/DroneDB/IDdbRaster.cs | Introduces role interface for raster operations. |
| Registry.Ports/DroneDB/IDdbMeta.cs | Introduces role interface for metadata operations. |
| Registry.Ports/DroneDB/IDdbIndex.cs | Introduces role interface for index operations including AddRawBatchWithOptions. |
| Registry.Ports/DroneDB/IDdbBuild.cs | Introduces role interface for build operations. |
| Registry.Ports/DroneDB/IDdbAnalytics.cs | Introduces role interface for analytics operations. |
| Registry.Ports/DroneDB/IDDB.cs | Converts IDDB into an aggregate over the new role interfaces while retaining constants. |
| Registry.Ports/DroneDB/BatchAddResult.cs | Adds DTOs representing batch-add completeness results (entries/unchanged/errors). |
| Registry.Adapters/DroneDB/NativeDdbWrapper.cs | Implements native P/Invoke entrypoint for batch add with options and throws DdbBusyException on contention. |
| Registry.Adapters/DroneDB/DdbResult.cs | Adds Busy result code for transient contention. |
| Registry.Adapters/DroneDB/DdbBusyException.cs | Adds typed exception to represent retryable DB contention. |
| Registry.Adapters/DroneDB/Ddb.cs | Preserves DdbBusyException through adapter layers and adds AddRawBatchWithOptions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Suppressed comments (4)
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:90
- The timeout and caller token only govern channel writes. Once all requests are queued,
Task.WhenAllignoreslinked, so a blocked native commit can wait forever and the advertisedEnqueueTimeoutSeconds/cancellation contract never fires. Apply the linked token while awaiting completion so the existing timeout catch can returnTransientException.
return await Task.WhenAll(tasks);
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:104
- This implementation returns immediately even when the dataset lane has queued or in-flight requests, contradicting the public contract that
FlushAsyncwaits for pending work to commit. A caller that starts enqueues and then flushes can therefore proceed before the index write. Add a per-lane barrier request (or track a drain generation/task) and complete it only after all preceding requests have committed.
public Task FlushAsync(DatasetKey dataset, CancellationToken ct = default)
{
// The drain loop commits everything currently queued on every iteration; enqueueing
// paths is what waits for a commit, so flushing is naturally expressed as enqueueing
// zero work and waiting for the lane to have drained at least once. Since there is
Registry.Web/Utilities/ApiExceptionFilter.cs:20
- The global filter does not handle the main upload failure path:
ObjectsController.Postcatches every exception atObjectsController.cs:603-609, andControllerBaseEx.ExceptionResultmapsDdbBusyException/TransientExceptionto its default 400 response. Consequently, contention from the new index queue never reaches this filter and clients receive 400 instead of the intended retryable 503. Update the existing exception helper (or let these typed exceptions escape controller catches) so the mapping is effective for existing actions.
/// Additive: existing controller-level try/catch + <see cref="ControllerBaseEx.ExceptionResult(Exception)"/>
/// still work as-is for actions that already handle their own errors; this filter only fires for
/// exceptions that escape the action unhandled.
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:251
- After the first busy result, the second attempt catches every exception and labels it transient. If that attempt fails with disk-full, corruption, an incompatible native library, or another permanent error, clients are incorrectly told to retry and the original classification is lost. Only a second
DdbBusyExceptionshould be wrapped asTransientException; other exceptions should be propagated to the batch requests unchanged.
catch (Exception retryEx)
{
var transient = new TransientException(
$"Dataset '{key.OrgSlug}/{key.InternalRef}' index is busy; retry the request", retryEx,
retryAfterSeconds: 2);
1. NativeDdbWrapper.Build: Handle DdbResult.Busy -> DdbBusyException (Hangfire retries) 2. DatasetIndexQueue.EnqueueAsync: Wrap for-loop + WriteAsync in try/catch for TransientException 3. ApiExceptionFilter: Use generic 500 message to prevent leaking ex.Message details 4. ObjectsManager.QuarantineAsync: Add GUID suffix (collision resistance) 5. ObjectsManager: Split IndexFileAsync + FinalizeIndexedFileAsync (fix #9 quarantine overreach)
The wrapper wrapped native Busy results in generic DdbException and the manager fell through to upload quarantine on transient contention, so Hangfire retry decorators (OnlyOn = DdbBusyException) never matched. Propagate DdbBusyException untouched and keep a healthy file out of quarantine when indexing hits transient contention.
ExceptionResult answered transient contention (TransientException, DdbBusyException) with the 400 default, which the front-end retry set [0,429,502,503,504] treats as terminal. Answer 503 with a Retry-After header instead, mirroring ApiExceptionFilter for the same exceptions.
A drain-loop reader failure discarded the pending batch, leaving callers awaiting Task.WhenAll forever; a non-positive tuning value would hang every enqueue or commit empty batches silently. Fault the batch TCSs with a TransientException and validate IndexQueueSettings at startup so both misbehaviors surface immediately. The hang-regression test needs the new AwaitWithin helper, which stays with it.
Pins the ClientApp submodule to 2ab331f (Improved dropzone resilience), which pairs with the backend busy/503 handling: the front-end now uploads and retries dropzones resiliently instead of failing on transient contention.
Requeue previously applied a DeletedState, which archived the job and could not re-run it. It now performs a same-id transition back to EnqueuedState, accepted only from a Failed live Hangfire state so a stale display-only JobIndex row cannot gate the transition. The unit tests pin the contract: requeue never deletes, missing or non-failed jobs are rejected without a state change, and Delete remains a separate terminal archive.
The retry endpoint could start a competing build run while the pending-build sweep already owned a retry via an on-disk .pending marker, and it requeued without clearing stale prior-run JobIndex state, which raced the async Enqueued stamp. Builds with a pending marker now get a 409 (sweep owns the retry, check failure never blocks), the row's stale error/timestamp/artifact fields are reset while it is still Failed, and a rejected requeue also surfaces a 409.
Centralize the native DdbResult-to-exception mapping in a public Testable DdbResultMapper so transient contention surfaces as DdbBusyException / DdbBuildInProgressException / DdbCanceledException instead of a generic DdbException (workstream 03 section 3.1). Every P/Invoke site in NativeDdbWrapper now routes through ThrowForFinalResult, and the Ddb adapter uses catch filters so the typed transient outcomes reach callers untouched. This is what makes the Hangfire retry decorators and the 503 + Retry-After client path actually fire. Also fixes AddWithOptions to a non-recursive, fixed conflict-retry policy and documents the intentional duplication of the per-method AutomaticRetry policy.
Introduce ApiExceptionClassifier as the single decision table for managed-exception-to-HTTP outcomes (status, Retry-After, NoRetry, message, log level) and delete the legacy per-action wrappers in ControllerBaseEx plus ~49 catch blocks spread across the controllers, which now let exceptions reach the global ApiExceptionFilter (phase D). The union table deliberately changes a few observable statuses: server bugs are 500 (no longer masked as 400), OperationCanceledException is 408, build-in-progress is 503 + Retry-After, and quota errors use their typed codes. ApiExceptionClassifierTests pins the table; the now-superseded ControllerBaseExExceptionResultTests is deleted.
The buffered AddNew path wrote the payload straight to the final dataset path with no compensation, leaving an untracked orphan on disk whenever indexing failed. Route it through the same stage-to-temp, atomic-move, index, quarantine flow as streamed uploads so every failure path is compensated (review round 2, finding 1). Move the .uploads / quarantine folder names into IDDB as the single source of truth shared by ObjectsManager and the reconciliation sweep, and make the quarantine step synchronous.
Per-dataset index lanes were long-lived singletons retained for the life of the process. Release the lane when a dataset is removed and idle-trim it after a configurable IdleLaneTrimSeconds (default 1800, validated > 0). A retired lane is transparently recreated by the next enqueue, and in-flight work still commits, so no write is lost. Replace no-op FlushAsync with Release on the queue port and call it from DatasetCleanupService before the dataset is deleted.
The reconciliation sweep used a recursive Directory.EnumerateFiles(AllDirectories) with only a post-filter, so at any depth it could recurse into the live .ddb database folder and surface staged/quarantined uploads as unindexed. Replace it with an iterative walk that prunes the .ddb and .uploads reserved folders during the traversal (review round 2), keeping IsReservedPath as a belt-and-braces post-filter and sourcing the folder names from IDDB.
Extract the NullIndexedEnqueuer IIndexedJobEnqueuer double (previously copy-pasted as a private nested class in RetryEndpointTest and BackgroundJobsProcessorTest) into a shared Registry.Web.Test.Adapters type, and add JobStorageScope, an RAII helper that swaps the static JobStorage.Current and restores the previous value on disposal (review round 2, finding 3/D4). Fix the JobStorage.Current leak between fixtures.
Set QT_LOGGING_RULES=*.warning=false on the three registry production services. A vendored Nexus/libnexus qWarning (QFSFileEngine::map beyond-file-size) floods stderr by hundreds of MB/h per service during builds; the wildcard is needed because qWarning writes to the default category, not a qt module category. Temporary workaround, removed once the upstream fix ships.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:145
- The configured deadline stops only
WriteAsync; once all requests fit in the channel,Task.WhenAlldoes not observelinked. A dead/stuck drain loop therefore leaves the HTTP caller waiting indefinitely instead of producing the documentedTransientException. Apply the linked token while awaiting completion.
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:341 - This catch classifies every second-attempt failure as transient. If the first attempt is busy but the retry reveals disk-full, corruption, or another permanent error, callers receive 503/retry guidance forever and the real failure is hidden. Only wrap a second
DdbBusyException; propagate other retry failures unchanged.
Registry.Web/Services/Adapters/IndexReconciliationService.cs:130 - This prunes any directory named
.ddbor.uploads, even below a normal user directory. Only root-level prefixes are reserved (IsReservedPathandObjectsManagerallow paths such asproject/.ddb/file.tif), so those valid files are omitted fromonDiskand may be falsely reported missing. Restrict pruning to direct children of the dataset root.
Registry.Web/Utilities/ApiExceptionClassifier.cs:77 - The default 500 arm also catches many existing client-validation failures. For example,
ObjectsManagerthrowsInvalidOperationExceptionfor reserved paths, missing source entries, same source/destination, and overwrite conflicts; these previously became 400 responses throughExceptionResult, but now become generic 500s. Migrate those validation sites toBadRequestException/ConflictException(rather than broadly mapping everyInvalidOperationExceptionto 400) before removing the legacy helper.
Registry.Adapters/DroneDB/DdbResultMapper.cs:26 DDBGetLastErrorcan return an empty string when no native call set an error (the new mapping test explicitly documents this), so the null-only fallback produces typed exceptions and API responses with no useful message. Treat empty/whitespace text as missing as well.
return GetNativeLastError() ?? (operation != null ? "Unknown error in " + operation : "Unknown error");
Registry.Web/Utilities/ApiExceptionFilter.cs:35
- Most migrated controller actions still catch, log at Error, and rethrow. This new global log therefore records the same exception a second time, and expected 4xx exceptions are still emitted once at Error despite the classifier selecting Information. Remove the redundant catch/log/rethrow blocks (or otherwise ensure a single logging owner) so the centralized level policy actually takes effect.
Registry.Web/Utilities/ApiExceptionClassifier.cs:58 - Correct “contentment” to “contention.”
Registry.Web/Services/Adapters/DatasetIndexQueue.cs:160 - Correct “retairs” to “retires.”
Lane retirement was racy and broken: - Replacing a retired lane used an unconditional TryRemove(key), which could evict a healthy lane already installed by a concurrent enqueuer, leaving two drain loops on one dataset and breaking the single-writer guarantee. - Idle trim was unreachable: the drain loop parked in WaitToReadAsync with no deadline, so the trim check at the loop top only ran after a commit that had just refreshed LastActivityTicks. - Release deregistered the lane but never completed the channel, so the drain loop parked forever and leaked a task and a bounded channel per released dataset. Retirement now closes the writer before draining, so requests that slip past the Retired check are committed by the retiring lane; enqueueers hitting a closed channel replay on a fresh lane. Adds the regression test EnqueueAsync_ConcurrentWithIdleTrim_LosesNoCompletion. PR: #353
Rejected remote credentials during import browsing threw InvalidOperationException, which the API classifier collapsed into a generic 500. They now surface as UnauthorizedException, classified as a 401 (no-retry) with a client-facing message; the constant is shared between the browse endpoints and the tests.
Points the repository at the task-monitor upgrade on DroneDB/Hub: refcounted poller with change events, keyboard guard, and dataset task lifecycle fixes.
Convert array and list construction (ToArray, new[], ToList) to C# 13 collection expressions, and drop redundant fully-qualified names on the same lines. No intended behavior change.
Per-dataset task business logic (authorization and ownership, cached listing, artifact resolution, retry sweep guard) moves out of the controller into a testable ITasksManager, leaving thin endpoint delegates. Inline status returns become managed exceptions mapped to the same HTTP codes by the global classifier. Shared JobIndex projections, artifact helpers and the build tool id are centralized so the per-dataset and admin task managers cannot drift apart.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 121 out of 121 changed files in this pull request and generated 13 comments.
Suppressed comments (2)
docker/production/docker-compose.yml:114
*.warning=falsedisables every Qt warning category, not only the noisy default-category message described above. This can hide unrelated production diagnostics. Scope the rule to the default category instead.
docker/production/docker-compose.yml:89*.warning=falsedisables every Qt warning category, not only the noisy default-category message described above. This can hide unrelated production diagnostics. Scope the rule to the default category instead.
Bumps the ClientApp submodule (30f3bd1 -> 7858299) to pick up:
- [BUG] Park retried upload files as ADDED during backoff so
processQueue() from other files cannot bypass Retry-After
- [BUG] Include errorType in the task poll snapshot signature
so cleared errors re-render the UI
- [BUG] Skip TaskHistory listener registration when unmounted
mid-mounted
- [CHG] Associate MaskBordersDialog source label with the input
Wiping prior-run state before the re-queue race the async Enqueued stamp and, when the re-queue was rejected (job purged or state moved), destroyed failure diagnostics that were still valid. Reset now runs only after Hangfire accepts the transition, and the guard also accepts a freshly Enqueued row while refusing live/terminal states. Issue: #353
A fresh attempt (Processing) or a recovery (Succeeded) superseded the transient error a prior attempt recorded (e.g. a DdbBusyException that was auto-retried to success), which otherwise kept showing in the task UI. Paired with the frontend signature change that tracks errorType so the cleared error re-renders. Issue: #353
The channel-write path was deadline-guarded but the final WhenAll await was not: once a batch was accepted, a stalled native write could hang the caller past the deadline. The await now observes the linked token, and on timeout/cancel the unawaited TCS completions are observed before rethrowing. Issue: #353
An explicit null lines array, null entries, or out-of-range Unix-ms timestamps in a stored LogTailJson would throw later in AsStrings and break a status or log response. Parse now normalizes them away, with tests covering fallback and entry filtering. Issue: #353
The OGC exception filter collapsed DdbBusyException and DdbBuildInProgressException into the generic 500 arm. They now map to 503 + Retry-After like the API pipeline's unified transient mapping in ApiExceptionClassifier. Issue: #353
Concurrent writes to the same dataset race on the SQLite writer lock, causing
database is lockedfailures and dropped index updates. This PR serializes index writes per dataset, maps nativeDdbResultfailures to typed exceptions, and retries transient contention instead of failing or leaving files half-indexed.Backend
DatasetIndexQueue: per-dataset coalescing queue: concurrent index requests for one dataset drain through a single lane as one native batch call with per-item isolation. Lanes released on dataset delete, idle-trimmed after a configurable period (IndexQueue:IdleLaneTrimSeconds, default 15 min), retirement race/leak fixed.NativeDdbWrappermapsDdbResultshapes via the newDdbResultMapper; newDdbBusyExceptionpropagates for retry instead of being swallowed asInvalidOperationException. Batch add returns per-itemBatchAddResult(success / unchanged / failed).ApiExceptionClassifier+ApiExceptionFiltergive all controllers one exception→HTTP mapping; transient DB contention →503+Retry-After.DeletedState). Retry endpoint returns409when a pending-build sweep already owns the retry, and resets stale error/artifact fields on requeue.AddNewstages to<org>/<ds>/.uploads/and quarantines toquarantine/on index failure instead of leaving orphan files. NewIndexReconciliationServicerecurring job re-queues on-disk files missing from the index, pruning.ddb,.uploadsandquarantineroots.IDDBsplit into role interfaces (IDdbIndex,IDdbBuild,IDdbMeta,IDdbRaster,IDdbAnalytics).DdbManager.Client (ClientApp submodule → Hub)
Resilient uploads: retry with AIMD adaptive concurrency and proper backoff, session teardown on permanent failure, dropzone hardening. Plus: reworked task progress bar with indeterminate state, and point clouds render without a valid CRS.
Tests
New suites:
DatasetIndexQueueTests,ApiExceptionClassifierTests,DdbBuildBusyPassthroughTests,IndexReconciliationServiceTests,NativeDdbWrapperResultMappingTests,BackgroundJobsProcessorTest,RetryEndpointTest(409 / same-id requeue contract), quarantine/AddNewcases inObjectManagerTest. Shared Hangfire test doubles extracted. All green:Registry.Web.Test971/971,Registry.Adapters.Ddb.Test88/88,Registry.Adapters.Test7/7.Behavior changes
503and the client retries; the server quarantines the file in the meantime.IDdbWrapper.AddWithOptionstrimmed to(ddbPath, paths, stopOnError).