Refactor task queue to resolve task-related failures - #964
Conversation
- Refactor task queue to resolve task-related failures - Remove priority and expiration time from task as unused - Add task timeout watchdog - Add name to queue for debugging purposes - Add revision condition to task Client and Repository API - Set default available time for a new task to now - Update runner deadline to return duration, not time - Add Prometheus metrics to Dexcom client - Add Prometheus metrics to task queue - Add serialization mutex to test logger - Require OAuth client to specify configured http client - Capture response metrics on all outgoing OAuth client requests - Add Prometheus helpers for outgoing clients - Add common CloseCursor function to properly close Mongo cursors and log errors - Add and update tests - Fix typos - https://tidepool.atlassian.net/browse/BACK-4523
- Make task deadline time database only - Add grace period to runner watchdog to remove spurious metrics - Minor updates
Code reviewFound 1 issue:
Lines 631 to 638 in 9d0e3b1 Lines 663 to 665 in 9d0e3b1 platform/task/store/mongo/mongo.go Lines 607 to 611 in 9d0e3b1 platform/services/tools/dexcom_analyze/dexcom_analyze.go Lines 93 to 96 in 9d0e3b1 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- Clear task available time appropriately - Add and update tests
|
Resolve code review issue above. |
There was a problem hiding this comment.
Pull request overview
This PR performs a broad refactor of the task queue/task repository to improve reliability under concurrent execution and shutdown, while also adding standardized Prometheus instrumentation for outgoing HTTP clients (Dexcom/Oura/Twiist) and a few supporting test/util updates.
Changes:
- Refactors task lifecycle and persistence with revision/state-lock based compare-and-swap, new start/stop transition APIs, and improved unstick/deadline behavior.
- Updates queue/runner contracts (deadlines as
time.Duration, watchdog/stop timeouts) and adds Prometheus metrics for queue health and runner behavior. - Introduces shared Prometheus RoundTripper helpers for outgoing HTTP requests and updates OAuth/provider clients to require an explicit configured
http.Client.
Reviewed changes
Copilot reviewed 76 out of 77 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| twiist/provider/provider.go | Adds Prometheus-instrumented HTTP client for Twiist OAuth/provider traffic |
| test/time.go | Exposes test.Now() helper |
| test/http/http.go | Adds a test RoundTripper for HTTP client tests |
| task/test/task.go | Adds task random generators aligned with new task fields (revision/state lock, etc.) |
| task/test/task_mocks.go | Updates gomock client signatures to include revision conditions |
| task/test/task_accessor.go | Removes legacy TaskAccessor test helper |
| task/test/client.go | Replaces embedded TaskAccessor with an explicit test Client implementing new condition-aware APIs |
| task/task.go | Refactors task models (remove priority/expiration, add revision/stateLock logging fields, default available time) and Client API condition support |
| task/task_test.go | Adds coverage for NewTask available-time defaulting and LogFields() |
| task/store/test/task_session.go | Removes unused TasksSession helper |
| task/store/store.go | Refactors repository interface: adds Start/Stop transitions, typed conditions, and unstick returning IDs |
| task/store/mongo/mongo.go | Implements new task repository semantics (CAS start/stop, revised unstick, iterate pending, metrics) |
| task/store/mongo/mongo_test.go | Updates mongo repository tests for new APIs and semantics |
| task/service/service/service.go | Updates service queue initialization to pass runners at MultiQueue construction |
| task/service/service/client.go | Maps request-level conditions to store-level conditions |
| task/service/api/v1/v1.go | Adds query-decoded request conditions to Get/Update/Delete task endpoints |
| task/queue/test/runner.go | Updates runner deadline API (duration) and adds multiple test runners for failure/timeout scenarios |
| task/queue/queue.go | Major queue refactor: single-use lifecycle, watchdog/metrics, CAS start/stop integration, shutdown behavior |
| task/queue/queue_test.go | Adds extensive coverage for queue lifecycle, shutdown, timeouts, panics, and CAS behavior |
| task/queue/queue_internal_test.go | Updates internal tests for iterator failure logging and computeState behavior |
| task/queue/multi.go | Refactors MultiQueue to build per-runner queues at construction and stop them concurrently |
| task/queue/multi_test.go | Updates tests to match new MultiQueue construction and runner registration model |
| task/client/client.go | Adds condition support for Get/Update/Delete and improves error wrapping |
| summary/task/updaterunner.go | Removes explicit AvailableTime set and updates deadline API to duration |
| summary/task/updaterunner_test.go | Updates expectations after AvailableTime defaulting change |
| summary/task/migrationrunner.go | Removes explicit AvailableTime set and updates deadline API to duration |
| summary/task/migrationrunner_test.go | Updates expectations after AvailableTime defaulting change |
| store/structured/mongo/result.go | Adds CloseCursor helper with timeout and logging |
| store/structured/mongo/result_test.go | Adds unit tests for CloseCursor plus import renames |
| store/structured/mongo/config.go | Fixes minor comment typo |
| store/structured/condition.go | Adds NewConditionWithRevision helper and mapping support |
| services/tools/dexcom_analyze/dexcom_analyze.go | Removes checks/resolutions for removed task fields (deadline/expiration) |
| request/inspector.go | Removes deprecated Prometheus response inspector utilities |
| request/condition.go | Adds NewConditionWithRevision helper |
| prometheus/test/prometheus.go | Adds Prometheus test helpers for metric lookup and label maps |
| pointer/default.go | Adds DefaultArray helper for slices |
| pointer/default_test.go | Adds tests for DefaultArray |
| plugin/abbott/go.sum | Adds github.com/gowebpki/jcs checksums and testify go.mod entry |
| plugin/abbott/go.mod | Adds indirect dependency github.com/gowebpki/jcs |
| oura/provider/provider.go | Adds Prometheus-instrumented HTTP client and path patterns for Oura requests |
| oura/provider/provider_test.go | Adds tests for Oura Prometheus path pattern generation |
| oura/oura.go | Adds DataTypeToPath helper to handle Oura path naming inconsistencies |
| oura/oura_test.go | Adds tests for DataTypeToPath |
| oura/client/client.go | Switches to oura.DataTypeToPath, removes old Prometheus inspector usage, increases request duration maximum |
| oura/client/client_test.go | Updates tests to use oura.DataTypeToPath and removes inspector-related tests |
| oauth/provider/provider.go | Requires explicit http.Client and wires it into oauth2 token source context |
| oauth/provider/client/client.go | Updates provider client constructors to accept explicit http.Client |
| log/test/serializer.go | Adds mutex to serialize access in test logger serializer |
| go.mod | Promotes prometheus/client_model to a direct dependency |
| ehr/sync/task.go | Removes explicit AvailableTime set (now defaulted in Task creation) |
| ehr/sync/task_test.go | Updates expectations after AvailableTime defaulting change |
| ehr/sync/runner.go | Updates deadline API to duration |
| ehr/reconcile/task.go | Removes explicit AvailableTime set |
| ehr/reconcile/task_test.go | Updates expectations after AvailableTime defaulting change |
| ehr/reconcile/runner.go | Updates deadline API and condition-aware task client calls |
| ehr/reconcile/runner_test.go | Updates mock expectations for condition-aware DeleteTask |
| dexcom/provider/provider.go | Adds Prometheus-instrumented HTTP client, request-time header metric, and condition-aware task deletion |
| dexcom/provider/provider_test.go | Adds tests for Dexcom request-time metric RoundTripper |
| dexcom/moment.go | Renames/adjusts compacting behavior to exclude moments without system time |
| dexcom/moment_test.go | Updates tests for new moments compaction semantics |
| dexcom/fetch/runner.go | Updates deadline API to duration |
| dexcom/fetch/runner_test.go | Updates deadline expectation accordingly |
| dexcom/event.go | Fixes incorrect Warnf usage when transmitter ID is empty |
| dexcom/egv.go | Fixes incorrect Warnf usage when transmitter ID is empty |
| dexcom/device.go | Fixes typo in comment (“specfied” → “specified”) |
| dexcom/data_range.go | Uses updated moments compaction function |
| dexcom/data_range_test.go | Adds tests for data range behavior excluding moments without system time |
| dexcom/client/client.go | Removes response inspector metrics and uses WarnIfDurationExceedsMaximum helper |
| dexcom/calibration.go | Fixes incorrect Warnf usage when transmitter ID is empty |
| dexcom/alert.go | Fixes incorrect Warnf usage when transmitter ID is empty |
| data/store/mongo/mongo_datum.go | Fixes comment typo (“the the”) |
| data/service/service/standard.go | Fixes log message typo (“the the”) |
| client/round_tripper.go | Adds generic RoundTripper wrapper with default-transport resolution |
| client/round_tripper_test.go | Adds tests for the generic RoundTripper wrapper |
| client/prometheus.go | Adds Prometheus URL path matching + request count/duration RoundTripper utilities |
| client/prometheus_test.go | Adds tests for Prometheus RoundTripper utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3ff73f9 to
8b90137
Compare
- Correctly wire up round tripper in OAuth clients - Remove unnecessary and outdated mocks - Update tests
- Update Prometheus metrics - Capture Dexcom response request-time header correctly - Warn and capture if task revision changed during run - Simplify task queue tests
- Add checks for task claims that have been lost - Update Dexcom task runner to correctly update last import time - Update Dexcom task runner to correctly reschedule - Fix delay constants - Update comments - Update tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change redesigns task scheduling and Mongo task lifecycle handling, adds conditional task operations, introduces HTTP and Prometheus instrumentation, exposes metrics endpoints, updates Dexcom and Oura integrations, and refreshes supporting tests, tooling, configuration, and documentation. ChangesTask contracts and orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TaskManager
participant TaskQueue
participant TaskRepository
participant Runner
TaskManager->>TaskRepository: IteratePending
TaskQueue->>TaskRepository: StartTask
TaskQueue->>Runner: Run with cancellation and watchdog
Runner-->>TaskQueue: terminal task state
TaskQueue->>TaskRepository: StopTask with claim token
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dexcom/fetch/runner.go (1)
406-437: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
importCompletedis never set on the deadline-exceeded path, soLastImportTimeis not persisted.In
fetchSinceLatestDataTime, the early return when the run exceedsGetRunnerDurationMaximum()(lines 427-430) does not sett.importCompleted = true. SinceupdateDataSourceWithTaskState(lines 296-300) only stampsLastImportTimewhenimportCompletedis true, data successfully fetched/stored just before hitting the deadline is never reflected inLastImportTimeon the data source.This contradicts the new tests in
dexcom/fetch/runner_test.go("is available soon if the deadline is exceeded" and "...and a later update fails"), both of which assertdataSrc.LastImportTimeis non-nil after this exact code path runs.🐛 Proposed fix
// If the task has been running for longer than the maximum duration, then stop fetching and repeat after a minute if time.Since(t.runTime) > t.GetRunnerDurationMaximum() { t.availableAfter = pointer.From(time.Minute) + t.importCompleted = true return nil }Also applies to: 296-300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dexcom/fetch/runner.go` around lines 406 - 437, Set t.importCompleted = true before the deadline-exceeded return in fetchSinceLatestDataTime, after the successfully fetched range is stored and before assigning availableAfter. Preserve the existing retry scheduling and ensure updateDataSourceWithTaskState can persist LastImportTime for partial imports.
🧹 Nitpick comments (11)
oauth/client/client_test.go (1)
195-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the injected HTTP-client path.
Line 197 always supplies
nil, and the mock only checks for a non-nil context. Add a case with a distinct*http.Clientand verifyHTTPClientreceives it viactx.Value(oauth2.HTTPClient).Also applies to: 238-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oauth/client/client_test.go` around lines 195 - 197, Add test coverage for the injected HTTP-client path in the oauthClient.New setup blocks, including the corresponding case around the second referenced setup. Pass a distinct *http.Client instead of nil, then verify the mock receives it through ctx.Value(oauth2.HTTPClient) while preserving the existing non-nil context validation.task/queue/queue_test.go (1)
495-495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest description grammar.
"logs a warning if the runner update the task while it was running"→updated;"logs a warning if a task that exceeds its maximum duration"→ dropif aor reword to"logs a warning when a task exceeds its maximum duration".Also applies to: 899-899
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/queue/queue_test.go` at line 495, Correct the grammar in the affected It test descriptions in the queue tests: update the runner wording to use “updated,” and reword the maximum-duration description to “logs a warning when a task exceeds its maximum duration.”task/queue/queue.go (1)
366-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWatchdog can double-count a single overrun run.
If a cooperative runner returns after
GetRunnerTimeout()+RunnerWatchdogGracePeriod, thetime.AfterFuncalready fired ("blocked") and the reconciliation at Line 460 also records"recovered", so one run contributes to both dispositions. If the intent is that dispositions are mutually exclusive, guard the recovered increment on whether the watchdog fired (e.g. capture theStop()return value).♻️ Sketch
- // Immediately stop the runner watchdog - runnerWatchdog.Stop() + // Immediately stop the runner watchdog; a false return means it already reported the run as blocked. + watchdogFired := !runnerWatchdog.Stop() @@ } else if cause := context.Cause(runnerContext); errors.Is(cause, ErrRunnerTimeoutExceeded) { lgr.Warn("Task runner exceeded timeout; task will be failed") tsk.AppendError(cause) - RunnerTimeoutExceededTotal.WithLabelValues(runner.GetRunnerType(), "recovered").Inc() + if !watchdogFired { + RunnerTimeoutExceededTotal.WithLabelValues(runner.GetRunnerType(), "recovered").Inc() + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/queue/queue.go` around lines 366 - 463, Update the runner watchdog handling in Queue.runTask so timeout dispositions are mutually exclusive: capture whether runnerWatchdog.Stop() successfully prevented the callback, and only increment the "recovered" RunnerTimeoutExceededTotal metric when the watchdog did not already fire. Preserve the existing "blocked" metric behavior for runs whose watchdog callback executes.task/test/task.go (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFailed fixtures contradict the new terminal-state invariant.
Task.SetFailednow clearsAvailableTime(task/task.goLine 291), and the PR objectives note that a failed task retainingavailableTimeis treated as anomalous.RandomTaskstill populatesAvailableTimeforTaskStateFailed(andTaskStateCompleted), so fixtures model a state production code no longer produces.♻️ Align failed/completed fixtures with the terminal-state invariant
case task.TaskStateFailed: - tsk.AvailableTime = pointer.From(test.RandomTimeFromRange(tsk.CreatedTime, now)) - tsk.ModifiedTime = pointer.From(test.RandomTimeFromRange(*tsk.AvailableTime, now)) + tsk.ModifiedTime = pointer.From(test.RandomTimeFromRange(tsk.CreatedTime, now)) tsk.Error = errorsTest.RandomSerializable()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/test/task.go` around lines 75 - 88, Update the TaskStateFailed and TaskStateCompleted branches in RandomTask so terminal-state fixtures leave AvailableTime unset, matching SetFailed’s invariant. Remove the AvailableTime assignments from both branches while preserving their existing ModifiedTime, error, runtime, duration, and deadline setup.task/store/mongo/mongo.go (1)
519-551: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUnfiltered
UnstickTaskscannot use the new partial index.The
{type, deadlineTime}partial index requires atypeprefix predicate, so whentypeFilteris nil (non-partitioned repository) this find degenerates to a collection scan overtasks. Fine today sinceMultiQueuealways partitions by type, but worth either documenting or adding a{deadlineTime}partial index for the unfiltered path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/store/mongo/mongo.go` around lines 519 - 551, Address the unfiltered UnstickTasks query path: when typeFilter is nil, ensure the repository has a suitable partial index on deadlineTime for running tasks, or document the intentional collection-scan behavior if that is the chosen design. Keep the existing {type, deadlineTime} index usage for filtered repositories and anchor the change around TaskRepository.UnstickTasks and its findSelector construction.client/config.go (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider accepting a duration string rather than bare seconds.
strconv.ParseIntrestricts config to whole seconds and drops the parse cause from the returned error.time.ParseDurationwould accept"30s","1m500ms", etc., and is the idiomatic choice for atime.Durationfield.♻️ Proposed refactor
- if timeoutString, err := reporter.Get("timeout"); err == nil { - if timeout, parseErr := strconv.ParseInt(timeoutString, 10, 0); parseErr != nil { - return errors.New("timeout is invalid") - } else { - c.Timeout = time.Duration(timeout) * time.Second - } - } + if timeoutString, err := reporter.Get("timeout"); err == nil { + timeout, parseErr := time.ParseDuration(timeoutString) + if parseErr != nil { + return errors.Wrap(parseErr, "timeout is invalid") + } + c.Timeout = timeout + }(
strconvimport then becomes unnecessary. Note this changes the expected config value format, so existing deployment values would need updating.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/config.go` around lines 44 - 50, Update the timeout parsing block to use time.ParseDuration instead of strconv.ParseInt, allowing values such as “30s” and “1m500ms” and assigning the parsed duration directly to c.Timeout. Preserve the existing invalid-timeout error behavior and remove the now-unused strconv import.client/prometheus.go (2)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese label names/values are effectively constants.
PrometheusLabelNameMethod/Path/StatusandPrometheusLabelValueErrorare exported mutable package vars; making themconstprevents accidental reassignment by importers (they are already referenced fromdexcom/clientand tests).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/prometheus.go` around lines 20 - 25, Change PrometheusLabelNameMethod, PrometheusLabelNamePath, PrometheusLabelNameStatus, and PrometheusLabelValueError from exported mutable variables to exported constants, preserving their existing names and string values so references in dexcom/client and tests continue to work.
92-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument duplicate metric name registration or make the registry explicit.
promauto.NewCounterVecandNewHistogramVecregister with the global default registerer and panic on duplicate metric names. Since this constructor is exported and named metrics are registered from package-level singletons, callers should be told it must be called once pernameat init; otherwise, accept a customprometheus.Registerer/auto.Registryso duplicate registration can be handled explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/prometheus.go` around lines 92 - 115, Update NewPrometheusRequestRoundTripperWithPathPatternsAndDurationBuckets and its wrapper to avoid undocumented duplicate registration: either document that each metric name may be constructed only once during initialization, or add an explicit prometheus.Registerer/auto.Registry parameter and register the CounterVec and HistogramVec through it so duplicate-registration errors can be handled without relying on the global default registerer.client/round_tripper.go (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
WithRoundTrippername implies a builder but mutates in place.
With…conventionally returns a new/derived value in Go. Since this mutates the receiver and returns nothing,SetRoundTrippercommunicates intent better. Also worth noting: mutating this after the round tripper is shared (these are package-level singletons indexcom/clientandoura/provider) is unsynchronized, so restrict use to setup/test wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/round_tripper.go` around lines 26 - 28, Rename the mutating RoundTripper.WithRoundTripper method to SetRoundTripper and update all callers accordingly. Keep its in-place assignment behavior, and ensure usage is limited to setup or test wiring rather than changing the shared round tripper after concurrent use begins.auth/service/api/v1/metrics.go (1)
17-21: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnchecked type assertion on the response writer.
res.(http.ResponseWriter)panics if the underlyingrest.ResponseWriterimplementation ever doesn't implementhttp.ResponseWriter(e.g. a wrapped/test double). A checked assertion with a 500 fallback keeps a metrics scrape from taking down the handler goroutine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/service/api/v1/metrics.go` around lines 17 - 21, Update Router.PrometheusMetrics to safely assert that res implements http.ResponseWriter before calling ServeHTTP; when the assertion fails, return an HTTP 500 response and avoid invoking the Prometheus handler, while preserving the existing metrics handling for valid writers.dexcom/client/client.go (1)
32-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon’t mutate the caller’s
Configand make the instrumented client carry Dexcom requests.
cfgis a caller-owned pointer, socfg.Timeout = 1 * time.Minutewrites defaulting into the caller after validation. Default on a local copy instead. Also, buildhttpClient.Timeoutfrom the configured timeout instead ofhttp.DefaultClient.Timeout, and make the DexcomTokenSourceimplementation return that same client fromHTTPClient(...), nothttp.DefaultClient, so the Prometheus round tripper is actually used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dexcom/client/client.go` around lines 32 - 41, Update the client construction around the Config defaulting and Dexcom TokenSource implementation: copy the caller-provided Config before applying the one-minute timeout default, set httpClient.Timeout from that local configured timeout, and make TokenSource.HTTPClient(...) return the instrumented httpClient instead of http.DefaultClient so Dexcom requests use prometheusRequestMetricsRoundTripper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/service/api/v1/metrics.go`:
- Around line 11-15: Update Router.MetricsRoutes and its route configuration so
GET /v1/metrics requires validation of the service secret before invoking
PrometheusMetrics. Ensure anonymous requests are rejected while preserving
authenticated metrics collection and the existing route behavior.
In `@client/client.go`:
- Around line 88-93: Update the request flow around createRequest and the
outbound httpClient.Do call so config.Timeout is applied to the request context
used by the HTTP transport. Remove the defer cancel in this function; invoke
cancel on every early-return/error path, and on success wrap the returned
io.ReadCloser so its Close method invokes cancel after streaming reads finish.
In `@README.md`:
- Line 204: Update the README description for
tidepool_task_type_lost_completion_total to use claim-token terminology instead
of state-lock, matching the metric behavior documented in
task/store/mongo/mongo.go.
- Around line 194-195: Update the descriptions of tidepool_task_workers_total
and tidepool_task_workers_available in the metrics documentation to remove the
hard-coded “5” worker count, and describe the per-queue values as determined by
deployment configuration.
In `@store/structured/mongo/result.go`:
- Around line 100-114: Update CloseCursor so nil contexts are replaced with
context.Background(), then always wrap the resulting context with
context.WithoutCancel and the existing 10-second context.WithTimeout before
calling cursor.Close. Preserve the current nil-cursor guard, cancellation
cleanup, and error logging behavior.
In `@task/store/mongo/mongo_test.go`:
- Around line 221-245: Update the timestamp tolerance assertions in the
“unsticks a running task with an expired deadline” test to compare against
wall-clock time from time.Now(), matching UnstickTasks’ UTC-based timestamps
instead of the frozen test.Now() value. Preserve the existing one-second
tolerance and the delayed available-time assertion in the adjacent test.
---
Outside diff comments:
In `@dexcom/fetch/runner.go`:
- Around line 406-437: Set t.importCompleted = true before the deadline-exceeded
return in fetchSinceLatestDataTime, after the successfully fetched range is
stored and before assigning availableAfter. Preserve the existing retry
scheduling and ensure updateDataSourceWithTaskState can persist LastImportTime
for partial imports.
---
Nitpick comments:
In `@auth/service/api/v1/metrics.go`:
- Around line 17-21: Update Router.PrometheusMetrics to safely assert that res
implements http.ResponseWriter before calling ServeHTTP; when the assertion
fails, return an HTTP 500 response and avoid invoking the Prometheus handler,
while preserving the existing metrics handling for valid writers.
In `@client/config.go`:
- Around line 44-50: Update the timeout parsing block to use time.ParseDuration
instead of strconv.ParseInt, allowing values such as “30s” and “1m500ms” and
assigning the parsed duration directly to c.Timeout. Preserve the existing
invalid-timeout error behavior and remove the now-unused strconv import.
In `@client/prometheus.go`:
- Around line 20-25: Change PrometheusLabelNameMethod, PrometheusLabelNamePath,
PrometheusLabelNameStatus, and PrometheusLabelValueError from exported mutable
variables to exported constants, preserving their existing names and string
values so references in dexcom/client and tests continue to work.
- Around line 92-115: Update
NewPrometheusRequestRoundTripperWithPathPatternsAndDurationBuckets and its
wrapper to avoid undocumented duplicate registration: either document that each
metric name may be constructed only once during initialization, or add an
explicit prometheus.Registerer/auto.Registry parameter and register the
CounterVec and HistogramVec through it so duplicate-registration errors can be
handled without relying on the global default registerer.
In `@client/round_tripper.go`:
- Around line 26-28: Rename the mutating RoundTripper.WithRoundTripper method to
SetRoundTripper and update all callers accordingly. Keep its in-place assignment
behavior, and ensure usage is limited to setup or test wiring rather than
changing the shared round tripper after concurrent use begins.
In `@dexcom/client/client.go`:
- Around line 32-41: Update the client construction around the Config defaulting
and Dexcom TokenSource implementation: copy the caller-provided Config before
applying the one-minute timeout default, set httpClient.Timeout from that local
configured timeout, and make TokenSource.HTTPClient(...) return the instrumented
httpClient instead of http.DefaultClient so Dexcom requests use
prometheusRequestMetricsRoundTripper.
In `@oauth/client/client_test.go`:
- Around line 195-197: Add test coverage for the injected HTTP-client path in
the oauthClient.New setup blocks, including the corresponding case around the
second referenced setup. Pass a distinct *http.Client instead of nil, then
verify the mock receives it through ctx.Value(oauth2.HTTPClient) while
preserving the existing non-nil context validation.
In `@task/queue/queue_test.go`:
- Line 495: Correct the grammar in the affected It test descriptions in the
queue tests: update the runner wording to use “updated,” and reword the
maximum-duration description to “logs a warning when a task exceeds its maximum
duration.”
In `@task/queue/queue.go`:
- Around line 366-463: Update the runner watchdog handling in Queue.runTask so
timeout dispositions are mutually exclusive: capture whether
runnerWatchdog.Stop() successfully prevented the callback, and only increment
the "recovered" RunnerTimeoutExceededTotal metric when the watchdog did not
already fire. Preserve the existing "blocked" metric behavior for runs whose
watchdog callback executes.
In `@task/store/mongo/mongo.go`:
- Around line 519-551: Address the unfiltered UnstickTasks query path: when
typeFilter is nil, ensure the repository has a suitable partial index on
deadlineTime for running tasks, or document the intentional collection-scan
behavior if that is the chosen design. Keep the existing {type, deadlineTime}
index usage for filtered repositories and anchor the change around
TaskRepository.UnstickTasks and its findSelector construction.
In `@task/test/task.go`:
- Around line 75-88: Update the TaskStateFailed and TaskStateCompleted branches
in RandomTask so terminal-state fixtures leave AvailableTime unset, matching
SetFailed’s invariant. Remove the AvailableTime assignments from both branches
while preserving their existing ModifiedTime, error, runtime, duration, and
deadline setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10cfe943-b178-4499-9192-9203463d0543
⛔ Files ignored due to path filters (1)
plugin/abbott/go.sumis excluded by!**/*.sum
📒 Files selected for processing (94)
.travis.ymlMakefileREADME.mdauth/service/api/v1/metrics.goauth/service/api/v1/router.goclient/client.goclient/config.goclient/prometheus.goclient/prometheus_test.goclient/round_tripper.goclient/round_tripper_test.godata/service/api/v1/metrics.godata/service/api/v1/v1.godata/service/service/standard.godata/store/mongo/mongo_datum.godexcom/alert.godexcom/calibration.godexcom/client/client.godexcom/client/client_test.godexcom/data_range.godexcom/data_range_test.godexcom/device.godexcom/egv.godexcom/event.godexcom/fetch/runner.godexcom/fetch/runner_test.godexcom/moment.godexcom/moment_test.godexcom/provider/provider.godexcom/provider/provider_test.goehr/reconcile/runner.goehr/reconcile/runner_test.goehr/reconcile/task.goehr/reconcile/task_test.goehr/sync/runner.goehr/sync/task.goehr/sync/task_test.goenv.shenv.test.sherrors/errors.goerrors/errors_test.gogo.modlog/test/serializer.gooauth/client/client.gooauth/client/client_test.gooauth/provider/client/client.gooauth/test/token_source.gooauth/test/token_source_source.gooura/client/client.gooura/client/client_test.gooura/oura.gooura/oura_test.gooura/provider/provider.gooura/provider/provider_test.goplugin/abbott/go.modpointer/default.gopointer/default_test.goprivate/plugin/abbottprometheus/test/prometheus.gorequest/condition.gorequest/inspector.goservices/tools/dexcom_analyze/dexcom_analyze.gostore/structured/condition.gostore/structured/mongo/config.gostore/structured/mongo/result.gostore/structured/mongo/result_test.gosummary/task/migrationrunner.gosummary/task/migrationrunner_test.gosummary/task/updaterunner.gosummary/task/updaterunner_test.gotask/client/client.gotask/queue/multi.gotask/queue/multi_test.gotask/queue/queue.gotask/queue/queue_internal_test.gotask/queue/queue_test.gotask/queue/runner.gotask/queue/test/runner.gotask/service/api/v1/v1.gotask/service/service/client.gotask/service/service/service.gotask/store/mongo/mongo.gotask/store/mongo/mongo_test.gotask/store/store.gotask/store/test/task_session.gotask/task.gotask/task_test.gotask/test/client.gotask/test/task.gotask/test/task_accessor.gotask/test/task_mocks.gotest/http/http.gotest/test.gotest/time.go
💤 Files with no reviewable changes (11)
- dexcom/provider/provider_test.go
- task/store/test/task_session.go
- ehr/reconcile/task_test.go
- task/test/task_accessor.go
- ehr/sync/task_test.go
- summary/task/migrationrunner_test.go
- oauth/test/token_source.go
- summary/task/updaterunner_test.go
- oauth/test/token_source_source.go
- request/inspector.go
- services/tools/dexcom_analyze/dexcom_analyze.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/client.go`:
- Around line 98-107: Synchronize the timeout callback and response completion
in the request flow around httpClient.Do: add a mutex-protected
response-received state, have the timer callback acquire the mutex and call
cancel only while Do is still pending, and mark the response as received under
the same mutex before stopping the timer. Update the relevant client method
without changing existing timeout behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fe07df0-f583-48b7-9093-1fecfe83529c
📒 Files selected for processing (5)
README.mdclient/client.goclient/client_test.gostore/structured/mongo/result.gotest/time.go
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
- Fix client duration parsing - Add generalized duration parsing - Add and update test
toddkazakov
left a comment
toddkazakov
left a comment
There was a problem hiding this comment.
Left two inline comments for your consideration
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
task/queue/queue.go (1)
437-448: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftExplicitly unset
AvailableTimefor terminal task updates.When a task completes or fails, the Mongo update must remove
AvailableTime. A nil update field does not remove the stale field from an existing document. New tasks haveAvailableTime, so failed tasks can retain it after this queue path persists the terminal state. This conflicts with Dexcom diagnostic tooling.Add an explicit unset operation to the task update contract and add a persistence test for both completed and failed tasks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/queue/queue.go` around lines 437 - 448, Update the task persistence contract used by the queue’s terminal-state updates to explicitly unset AvailableTime rather than relying on a nil field value. Ensure both completed and failed task updates remove any existing AvailableTime in Mongo, and add persistence coverage for both terminal outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@task/queue/queue.go`:
- Around line 437-448: Update the task persistence contract used by the queue’s
terminal-state updates to explicitly unset AvailableTime rather than relying on
a nil field value. Ensure both completed and failed task updates remove any
existing AvailableTime in Mongo, and add persistence coverage for both terminal
outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5e52715-5754-4527-af59-869fbf2569d5
📒 Files selected for processing (13)
auth/client/external.goauth/service/service/client.goclient/config.goclient/config_test.godexcom/fetch/runner.godexcom/fetch/runner_test.goduration/duration.goduration/duration_suite_test.goduration/duration_test.goservice/server/config.gotask/queue/queue.gotask/queue/runner.gotask/task.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/config.go
- task/task.go
|
@toddkazakov Just need a final review on the last two commits I just added - https://github.com/tidepool-org/platform/pull/964/changes/ca3d2b0faffea9bc06408065a21c6af7c306c0dd..355b240bdbcdde5b3dc1630f40baa69e747ac385. |
Uh oh!
There was an error while loading. Please reload this page.