fix: batch API endpoint exports for large history imports#86
Conversation
Split selected API export date ranges into bounded sequential batches (7 calendar days by default) instead of uploading one oversized envelope. Batches upload serially; the first failed batch stops the run and reports its date range while preserving success accounting from prior batches. Small exports and the scheduled two-day lookback are unaffected (single batch).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 244fec2af8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let batchSpan = max(1, maxBatchDaySpan) | ||
| let batches: [[Date]] = stride(from: 0, to: normalizedDates.count, by: batchSpan).map { | ||
| Array(normalizedDates[$0..<min($0 + batchSpan, normalizedDates.count)]) | ||
| } |
There was a problem hiding this comment.
The batching is not used by the manual history-export flow that needs it: the only production caller of APIEndpointExportRunner.export is the scheduled path at HealthMd/iOS/SchedulingManager.swift:698, while exportDataToAPIEndpoint still accumulates the entire selected range and calls APIExportClient.upload once at HealthMd/iOS/ContentView.swift:951. Consequently, selecting a large historical range in the app still creates the original oversized request and can hit the same timeout or request-size limit this change is intended to fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks — this was the real gap. ContentView.exportDataToAPIEndpoint now calls APIEndpointExportRunner.export instead of duplicating the fetch loop and calling APIExportClient.upload once directly. Added an onProgress callback to the runner so the manual UI keeps its per-date progress bar across what may now be several batch uploads. See 32c3ecd/d765f9b.
| allFailedDateDetails.append(FailedDateDetail( | ||
| date: batchStart, | ||
| reason: .fileWriteError, | ||
| errorDetails: "Batch upload failed for \(rangeDescription(start: batchStart, end: batchEnd)): \(error.localizedDescription)" | ||
| )) |
There was a problem hiding this comment.
When a later batch upload fails, this records only batchStart and then returns, so the other records in that failed batch and every date in subsequent unattempted batches are absent from failedDateDetails. For example, if batch 2 of a 15-day export fails, the result reports 7 successes and only day 8 as failed even though days 9–15 were not delivered; consumers such as the failed-date history/retry UI operate on this array and therefore cannot identify all missing dates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — on a batch upload failure we now record a FailedDateDetail for every date whose record was in that failed batch, plus every date in every batch after it (marked as "not attempted" rather than "failed", since they were never sent). Added testBatchFailureStopsSubsequentBatchesAndPreservesPriorSuccess assertions covering all 8 remaining dates (batch start through the untried trailing batch).
| guard !batchRecords.isEmpty else { | ||
| // Nothing to upload for this batch; continue to the next one | ||
| // so a single sparse batch doesn't abort the whole range. | ||
| continue |
There was a problem hiding this comment.
When every date in one batch has no readable data or a fetch error but a later batch contains records, this guard drops the first batch without sending its failed_date_details anywhere. Previously, a mixed range with at least one record produced one envelope containing failures for all other requested dates; now a scheduled lookback spanning multiple batches can leave the receiver unable to distinguish those dates from dates that were never requested. Send a failure-only envelope or otherwise carry these details to the endpoint without losing their range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by carrying a failure-only batch's failed_date_details forward and attaching them to the next batch that does upload successfully, so the endpoint still receives them (matching the pre-batching single-envelope behavior). If no later batch succeeds, they're flushed via a dedicated failure-only upload call at the end — but only if at least one batch succeeded overall, so an entirely-empty range still sends nothing (unchanged prior behavior). Covered by new tests testFailureOnlyBatchIsCarriedForwardToNextSuccessfulUpload and testTrailingFailureOnlyBatchIsFlushedInDedicatedUpload.
| allFailedDateDetails.append(contentsOf: batchFailedDateDetails) | ||
| totalExternalRecordCount += batchExternalRecords.count |
There was a problem hiding this comment.
This increments totalExternalRecordCount before the upload succeeds, so when a later batch is rejected the returned partial-success result includes external records from that undelivered batch. ExportResult.totalFilesWritten incorporates this field and scheduled exports persist that value through ExportOrchestrator.recordResult, causing history/accounting to report provider sidecars as exported even though the endpoint never received them; move the increment to the successful-upload path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — totalExternalRecordCount is now only incremented inside the successful-upload branch, not when the records are fetched. Added testExternalRecordCountOnlyIncludesSuccessfullyUploadedBatches (single-day batches, second batch rejected) asserting externalRecordFileCount reflects only the delivered batch.
…counting Addresses review feedback on CodyBontecou#86: - Route the manual export flow (ContentView.exportDataToAPIEndpoint) through APIEndpointExportRunner.export instead of duplicating the fetch/upload loop and calling APIExportClient directly. This was the actual production bug this PR needed to fix -- the scheduled path already used the runner, but manual history exports (the common case for large ranges) still built one oversized request. - Added a ProgressHandler to APIEndpointExportRunner.export so the manual UI can still show per-date progress across a multi-batch run. - When a batch upload fails, report every date in that batch's records (not just the batch start date) plus every date in every not-yet-attempted subsequent batch, so failed-date consumers see the full picture instead of a single date. - Carry a batch's failed-date details forward to the next batch that does upload successfully (instead of dropping them), so the endpoint still learns about failure-only batches. If no later batch succeeds, flush them in a dedicated failure-only request -- but only if at least one batch did upload; an entirely-empty range still sends nothing, matching prior behavior. - Only add a batch's external/provider record count to the result after its upload actually succeeds, not when the records are fetched, so a rejected batch's provider records aren't counted as delivered. New tests: testFailureOnlyBatchIsCarriedForwardToNextSuccessfulUpload, testTrailingFailureOnlyBatchIsFlushedInDedicatedUpload, testExternalRecordCountOnlyIncludesSuccessfullyUploadedBatches, and expanded assertions in testBatchFailureStopsSubsequentBatchesAndPreservesPriorSuccess to check every skipped date is reported.
ContentView.exportDataToAPIEndpoint previously duplicated the per-date fetch loop and called APIExportClient.upload directly with the whole range in one request -- the batching added to APIEndpointExportRunner never applied to this path, which is the one users actually hit when exporting a large history range from the app. It now calls APIEndpointExportRunner.export (with an onProgress callback preserving the existing per-date progress UI) and maps the returned ExportResult to status messages the same way exportLocalData already does for the vault target. apiExportFailureReason is still used by the Mac export streaming path (streamConnectedMacExport), so it's kept in place.
…eholder by mistake)
|
Pushed fixes for all four review comments (replies inline on each thread):
Added tests: Note: I pushed one bad intermediate commit to |
|
Thanks for submitting the issue and pr - let me look over it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d765f9b1d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ExportOrchestrator.ExportResult( | ||
| successCount: records.count, | ||
| successCount: totalSuccessCount, | ||
| totalCount: normalizedDates.count, |
There was a problem hiding this comment.
When a scheduled lookback exceeds seven days and a later batch upload fails, this returns a nonzero successCount from earlier batches. The checked scheduled path passes this result to ScheduledExportCoordinator.completePendingScheduledExport, whose successCount > 0 branch clears the entire pending request, while SchedulingManager also advances the last-export date; consequently, the failed and unattempted dates are not automatically retried even though they now appear in failedDateDetails.
Useful? React with 👍 / 👎.
| if Task.isCancelled { | ||
| return ExportOrchestrator.ExportResult( | ||
| successCount: totalSuccessCount + batchRecords.count, | ||
| totalCount: normalizedDates.count, | ||
| failedDateDetails: allFailedDateDetails + pendingUnsentFailedDateDetails + batchFailedDateDetails, |
There was a problem hiding this comment.
If the user cancels after one or more records have been fetched in the current batch but before that batch reaches the upload call, batchRecords.count is added to the success count even though those records never left the device. ContentView persists this result, consumes export quota, and reports those days as “uploaded,” while they are absent from the failed-date list used for retries; cancellation should count only previously completed uploads.
Useful? React with 👍 / 👎.
| _ = try await upload( | ||
| batchRecords, | ||
| combinedFailedDateDetailsForUpload, | ||
| batchExternalRecords, | ||
| settings, | ||
| apiSettings, | ||
| batchStart, | ||
| batchEnd |
There was a problem hiding this comment.
When an all-empty batch precedes a batch containing records, combinedFailedDateDetailsForUpload includes dates from that earlier batch, but the envelope range is still passed as the current batchStart...batchEnd. The endpoint therefore receives failed_date_details outside the request's declared date_range, which can cause range-validating consumers to reject or disregard those failures; the range must also cover the carried dates.
Useful? React with 👍 / 👎.
| allFailedDateDetails.append(contentsOf: pendingUnsentFailedDateDetails) | ||
| pendingUnsentFailedDateDetails = [] |
There was a problem hiding this comment.
For a successful batch followed by a trailing batch with no deliverable records, line 208 has already appended that batch's failures to allFailedDateDetails while also retaining them in pendingUnsentFailedDateDetails; appending the pending array again here records every trailing failure twice. Export history then displays duplicate dates, and its retry flow maps the duplicates directly into repeated export attempts.
Useful? React with 👍 / 👎.
| } catch { | ||
| allFailedDateDetails.append(contentsOf: batchFailedDateDetails) | ||
|
|
||
| // Every date whose record was part of this failed upload is | ||
| // itself undelivered — report each one, not just the batch | ||
| // start, so consumers (e.g. retry UI) can see exactly which | ||
| // dates need to be re-sent. | ||
| let failureMessage = "Batch upload failed for \(rangeDescription(start: batchStart, end: batchEnd)): \(error.localizedDescription)" | ||
| allFailedDateDetails.append(contentsOf: batchRecords.map { | ||
| FailedDateDetail(date: $0.date, reason: .fileWriteError, errorDetails: failureMessage) |
There was a problem hiding this comment.
If a batch contains both a no-data date and records whose upload is rejected, the per-date .noHealthData details are appended before the .fileWriteError details created for the rejected POST. Because ExportResult.primaryFailureReason and the manual failure dialog use the first detail, a 401/413/500 failure is presented as “No Health Data,” hiding the actionable endpoint error from the user; place the upload failure first or otherwise select it as primary.
Useful? React with 👍 / 👎.
| let flushStart = pendingUnsentFailedDateDetails.map(\.date).min() ?? dateRangeStart | ||
| let flushEnd = pendingUnsentFailedDateDetails.map(\.date).max() ?? dateRangeEnd | ||
| do { | ||
| _ = try await upload([], pendingUnsentFailedDateDetails, [], settings, apiSettings, flushStart, flushEnd) |
There was a problem hiding this comment.
When earlier batches succeeded but the trailing batch contains only failed dates, this sends a healthmd.api_export envelope with records: [] and record_count: 0. The public contract in docs/reference/index.md:30 defines this envelope as containing one or more daily records, and prior behavior deliberately skipped uploads when no records existed, so existing receivers may reject this new failure-only request; reporting failures without records requires an explicitly supported/versioned envelope contract or a different delivery mechanism.
Useful? React with 👍 / 👎.
|
I implemented the five review fixes and pushed them as a one-commit patch. Because maintainer edits are disabled on this fork, I could not write directly to this PR branch. Please merge Just-Insane#1 into Commit: Validation completed locally:
|
|
Follow-up: an independent audit found no remaining production-path defects. I added extra regression coverage for cancellation after an earlier successful batch, failure-detail uniqueness, serialized range scoping, generic transport-error redaction, silent-push partial batches, and Today Refresh markers. Patch PR Just-Insane#1 now includes commit |
|
Patch PR follow-up Validation after the fix:
Requested a fresh Codex review on Just-Insane#1. |
Address API export review findings
|
Merged your fixes into this PR's branch on my fork |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d5c95517
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| self.successCount = successCount | ||
| self.totalCount = totalCount | ||
| self.completedDateCount = completedDateCount ?? successCount |
There was a problem hiding this comment.
When a scheduled local-folder or Connected Mac export succeeds for some dates but reports .noHealthData for another, those existing producers omit completedDateCount, so this default sets it below totalCount. The new coordinator and scheduling checks then preserve the pending request and do not advance the schedule, causing the same successful dates and permanently empty date to be exported repeatedly. Either these producers must count terminal no-data outcomes as completed or the new retry decision must distinguish them from retryable failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 644a6ce1 (patch PR Just-Insane#2). ExportResult now carries exact terminal dates. iPhone-folder no-data outcomes complete, retryable HealthKit/device-lock failures remain pending, Connected Mac reports exact source-device dates, and legacy Mac peers are capability-gated. Residual requests contain only unresolved dates.
| batchFailedDateDetails, | ||
| batchExternalRecords, | ||
| settings, | ||
| apiSettings, |
There was a problem hiding this comment.
During a manual multi-batch export, apiSettings is the live mutable @StateObject, and the endpoint settings sheet remains reachable while the async export yields between batches. If the user edits the endpoint or token after an earlier batch uploads, subsequent batches read the changed values here and can send portions of the same health export to a different server or account. Capture an immutable endpoint and authorization snapshot when the export starts and use it for every batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 644a6ce1 (patch PR Just-Insane#2). Manual API export captures an immutable APIExportDestinationSnapshot before the task starts; every batch and the history label use the same endpoint, authorization value, and display name. Regression coverage mutates live settings after batch 1 and verifies batch 2 still uses the original snapshot.
| } else { | ||
| logger.info("Scheduled export partially completed; pending dates preserved for retry") | ||
| } | ||
| await sendExportNotification(success: true, daysExported: result.successCount) |
There was a problem hiding this comment.
When an earlier API batch succeeds but a later batch fails, this branch correctly recognizes that pending dates remain, but it immediately sends success: true. sendExportNotification renders that as “Export Completed” and “Successfully exported …,” so a background user receives no indication that part of the requested range failed and remains pending. Use a partial/failure notification whenever didCompleteAllRequestedDates is false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 644a6ce1 (patch PR Just-Insane#2). Incomplete automatic runs no longer post “Export Completed.” The coordinator persists a reduced stable-ID pending request and sends the actionable “Health Export Needs Attention” notification, whose payload retries only unresolved dates.
|
Follow-up fixes for the latest review are ready in Just-Insane#2 ( Highlights:
Validation on the final tree:
No daily export-schema change; sync additions are optional/capability-negotiated. |
|
Added |
|
Final follow-up is ready in Just-Insane#2 at |
Fix exact retries and add resumable corpus exports
There was a problem hiding this comment.
💡 Codex Review
Reject oversized Shortcut lookbacks before expanding
When a Shortcut supplies an extremely large days value now that the upper bound was removed, this date calculation can return nil (or otherwise leads to materializing an enormous [Date] on the main actor), and the force unwrap crashes the intent before it can report a validation error. Keep a practical cap or gracefully reject calendar overflow before expanding the requested range.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ = iphoneExportRequestCoordinator.complete(with: fileResult) | ||
| syncService.send(.macExportResult(fileResult)) | ||
| } | ||
| syncService.send(.connectedCorpusTransferFinalAck(acknowledgement)) |
There was a problem hiding this comment.
If the Mac finishes a partitioned strict-raw export, persists the terminal ACK, and the iPhone disconnects before receiving that ACK, a reconnect reaches this replay branch with fileResult == nil because strict-raw finalization never stores a terminalResult. The code then sends only connectedCorpusTransferFinalAck, leaving the active CLI/control request unresolved until its inactivity timeout instead of returning the already-finalized raw response; replay needs to complete or fail the pending raw control request as well.
Useful? React with 👍 / 👎.
| case .macHealthDay: | ||
| try await applyHealthDay( |
There was a problem hiding this comment.
For a strict-raw corpus request, requestExport skips Mac destination preflight because raw mode should not write files, but this switch applies any completed segment by its declared kind. If a malformed partition in a .strictRaw session carries a .macHealthDay item, this branch calls applyHealthDay and can write daily files to the vault; validate segment.kind against session.journal.exportManifest.mode before applying.
Useful? React with 👍 / 👎.
| session.journal.failedDateDetails.append(FailedDateDetail( | ||
| date: payload.sourceDate, | ||
| reason: .fileWriteError, | ||
| errorDetails: error.localizedDescription | ||
| )) |
There was a problem hiding this comment.
Fresh evidence is the new corpus receiver path: the iPhone sends a successfully fetched but empty HealthData as record with failure == nil, and VaultManager.exportHealthData throws ExportError.noHealthData; this catch maps that to .fileWriteError and never adds the date to completedDates. In scheduled partitioned Connected Mac exports, permanently empty days therefore remain pending and are retried as write failures instead of being cleared as terminal no-data outcomes; handle ExportError.noHealthData separately here.
Useful? React with 👍 / 👎.
| session.journal.state = .cancelled | ||
| session.journal.updatedAt = Date() | ||
| try? persist(session) |
There was a problem hiding this comment.
If the Mac has already finalized and persisted a corpus session but the iPhone misses the final ACK and later sends connectedCorpusTransferCancel, this path restores the completed journal and unconditionally changes it to .cancelled. That can overwrite a successful file export with a cancelled result (or destroy the replayable strict-raw terminal ACK) even though the files/raw response were already finalized; completed sessions should replay or reject cancellation instead of being downgraded.
Useful? React with 👍 / 👎.
2 participants
Closes #85.
Problem
API Endpoint export built one
healthmd.api_exportJSON envelope for the entire selected date range and POSTed it in a single request. Large historical exports (e.g. ~7 months of granular HealthKit data) could exceed Cloudflare's request limits or time out.Change
APIEndpointExportRunner.exportnow splits the normalized, sorted date range into bounded sequential batches before uploading:APIEndpointExportRunner.defaultMaxBatchDaySpan = 7(calendar days), passed as a defaultmaxBatchDaySpanparameter so it can later become user-configurable without touching the batching algorithm.maxBatchDaySpandays.uploadclosure once per batch with that batch's owndate_range.start,date_range.end, records, and failed-date details.APIExportClientrequired no changes — it already builds the envelope from whatever range/records it's given, so each batch produces a correctly-scoped envelope automatically.FailedDateDetailis appended describing the failed date range (yyyy-MM-dd to yyyy-MM-dd) and the underlying error'slocalizedDescription— never the health payload or theAuthorizationheader value, since the existing error types never carry those.successCountaccumulates per successfully-uploaded batch, so a later failure does not erase credit for earlier successful batches.No server-side protocol changes; the wire format per request is identical to before, just scoped to at most 7 days per request.
Tests
Added to
HealthMdTests/Managers/APIEndpointExportRunnerTests.swift:testFifteenDayRangeSplitsIntoThreeSequentialSevenDayBatches— 15 consecutive days produce exactly 3 upload calls with 7, 7, and 1 day(s) respectively; asserts each call's date range and record set matches its batch.testBatchFailureStopsSubsequentBatchesAndPreservesPriorSuccess— forces the second of three batches to throw; asserts the third batch's upload closure is never invoked,successCountreflects only the first (successful) batch, and a.fileWriteErrordetail mentioning the failure is present.testEmptyDatesReturnsZeroCountsandtestSingleDayWithNoDataReportsNoHealthDataAndSkipsUpload— explicit coverage that empty-input and single-day/no-data behavior is unchanged, alongside the pre-existingtestUploadsOnlyDatesWithDataAndReportsEmptyDates,testProviderOnlyDayRemainsSupplementalAndDoesNotTriggerUpload, andtestUploadFailureReturnsFileWriteFailureWithoutCountingPreparedRecordsAsSuccess, all still passing unmodified.Test command:
I don't have a macOS/Xcode toolchain in this environment to execute the build, so I have not run this locally — please run the target and confirm before merging. All new/edited tests only exercise the injected
fetchHealthData/uploadclosures (no HealthKit or network access), so they should run in a standard simulator/CI job without special entitlements.Docs
Updated
docs/features/api-endpoint-export.md:APIEndpointExportRunnerandAPIExportClient.Scope
Only
HealthMd/Shared/Managers/APIEndpointExportRunner.swift, its test file, and the feature doc changed.APIExportClient.swiftis untouched — it already accepted per-call ranges/records, which is what made per-batch envelopes possible without protocol changes.