Skip to content

GET /health computes airdrop_expiry job health but never folds it into the aggregate status field — a dead reconciliation job is invisible to monitoring #123

Description

@prodbycorne

Overview

GET /health's top-level aggregate status field — the one value any external monitor, load balancer, or Kubernetes liveness/readiness probe would reasonably check — is computed from only two of the three leader-elected background jobs. The third, airdrop_expiry, has its detailed health computed and included in the response body, but is never folded into the aggregate status calculation at all.

const priceRefreshHealth = wrappedPriceRefreshJob.getHealth();
const webhookWorkerHealth = wrappedWebhookRetryWorker.getHealth();
const airdropExpiryHealth = wrappedAirdropExpiryJob.getHealth();

let status = 'ok';
if (!redisConnected || !priceRefreshHealth.healthy || !webhookWorkerHealth.healthy) {
  const jobsDegraded =
    (!priceRefreshHealth.healthy && !priceRefreshHealth.stalled) ||
    (!webhookWorkerHealth.healthy && !webhookWorkerHealth.stalled);
  status = (!redisConnected || priceRefreshHealth.stalled || webhookWorkerHealth.stalled)
    ? 'unhealthy'
    : jobsDegraded ? 'degraded' : 'unhealthy';
}

res.json({
  status,
  ...
  jobs: {
    price_refresh: { healthy: priceRefreshHealth.healthy, ... },
    webhook_retry_worker: { healthy: webhookWorkerHealth.healthy, ... },
    airdrop_expiry: { healthy: airdropExpiryHealth.healthy, stalled: airdropExpiryHealth.stalled, ... },
  },
  ...
});

airdropExpiryHealth is computed via wrappedAirdropExpiryJob.getHealth() and faithfully reported under jobs.airdrop_expiry in the response body — but the if (!redisConnected || !priceRefreshHealth.healthy || !webhookWorkerHealth.healthy) condition that gates the entire status computation references only redisConnected, priceRefreshHealth, and webhookWorkerHealth. airdropExpiryHealth never appears in that condition, nor in the jobsDegraded computation, nor in the inner ternary. If the airdrop-expiry reconciliation job stalls or dies — for example, if Horizon becomes persistently unreachable (the exact failure mode its own tick() function explicitly anticipates and logs a warning for), or if it's affected by the leader-election lifecycle bug described in a companion issue in this batch — GET /health's top-level status field will report 'ok' regardless, forever, as long as Redis and the other two jobs are fine.

This is precisely the failure mode a health-check aggregate exists to catch, and precisely the field any external system is most likely to check in isolation (nobody wires a liveness probe to parse jobs.airdrop_expiry.healthy out of a nested JSON body; they check the top-level status string, or an HTTP status code derived from it). A completely dead airdrop-expiry job — meaning airdrops silently never auto-expire against expiry_ledger again, silently undoing the entire point of already-closed issue #88 — would be invisible to any monitoring built against this endpoint's primary signal, even though the detailed data proving it's broken is sitting right there in the same response, one level down, unused by the very logic whose job is to summarize it.

Requirements

  • Include airdropExpiryHealth in the status/jobsDegraded aggregate computation on equal footing with priceRefreshHealth and webhookWorkerHealth.
  • Add a regression test that specifically stalls/fails only the airdrop-expiry job (leaving Redis, price-refresh, and webhook-retry healthy) and asserts the top-level status reflects that degradation — this is the exact scenario the current code fails silently on, and the one a test needs to target directly rather than only testing the two jobs that already participate in the aggregate.
  • While making this change, consider extracting the per-job aggregation logic into a small, named helper function that takes an array of { name, health } pairs, rather than a hand-written boolean expression enumerating each job by name — this bug is exactly the class of mistake ("added a third job but the boolean expression still only mentions two") that a loop over a list, rather than a hardcoded expression, would have made structurally impossible to reintroduce the next time a fourth leader-elected job is added.

Acceptance Criteria

  • GET /health's top-level status field changes (to 'degraded' or 'unhealthy' per the existing semantics) when airdrop_expiry's health is unhealthy/stalled, even when Redis, price-refresh, and webhook-retry-worker are all healthy.
  • A test explicitly covers this scenario (mocking/stubbing wrappedAirdropExpiryJob.getHealth() to return an unhealthy/stalled state while the other two jobs report healthy) and asserts the aggregate status is not 'ok'.
  • The fix does not regress the existing two-job aggregation behavior already covered by test/health.test.js.
  • (Recommended, not required) The per-job aggregation is refactored into a form that iterates over all registered jobs rather than naming each one explicitly in the boolean expression, to prevent this specific class of regression when a future job is added.

Additional Notes

More precise references

  • src/index.js /health handler: confirmed airdropExpiryHealth = wrappedAirdropExpiryJob.getHealth(); is computed (alongside priceRefreshHealth/webhookWorkerHealth) before the status computation begins.
  • Confirmed the status computation's if condition and the nested jobsDegraded boolean each reference priceRefreshHealth/webhookWorkerHealth/redisConnected explicitly by name, and neither references airdropExpiryHealth anywhere.
  • Confirmed jobs.airdrop_expiry is included in the JSON response body with healthy, stalled, last_error, leader, etc. — i.e. the data needed to fix this is already computed and available at the point the status variable is assigned; this is purely a "forgot to include it in the boolean expression" gap, not a missing-data gap.

Additional edge cases

  • Because leader-elected, non-leader instances report their own jobs as healthy: false by design (per the explicit comment in src/index.js: "a non-leader instance reports its jobs as not healthy... but that's expected — the leader is doing the work... distinguishes 'not leader' from 'stalled' via the leader field"), any fix here needs to preserve that same not-leader-vs-actually-stalled distinction for airdrop_expiry specifically — i.e. don't naively treat !airdropExpiryHealth.healthy as unconditionally bad; use airdropExpiryHealth.stalled the same way the existing two jobs do, consistent with the existing pattern, not a new one.
  • This gap compounds with the leader-election non-reentrancy issue described in a companion issue in this batch: if leaderAwareJob.js's accumulating-wrapper bug ever caused airdrop_expiry specifically to misbehave after a restart/reconfiguration cycle, this health-check gap is exactly what would prevent that from ever being noticed via /health.

Test/reproduction plan

jest.spyOn(wrappedAirdropExpiryJob, 'getHealth').mockReturnValue({ healthy: false, stalled: true, lastError: 'Horizon unreachable', lastSuccessAt: null });
jest.spyOn(wrappedPriceRefreshJob, 'getHealth').mockReturnValue({ healthy: true, stalled: false });
jest.spyOn(wrappedWebhookRetryWorker, 'getHealth').mockReturnValue({ healthy: true, stalled: false });
const res = await request(app).get('/health');
expect(res.body.status).not.toBe('ok'); // currently fails — status stays 'ok'

Cross-references

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial CampaignCampaign: Official CampaignOfficial Campaign | FWC26Campaign: Official Campaign | FWC26Third CampaignCampaign: Third CampaignbugSomething isn't workingobservabilityLogging, metrics, tracing, monitoringvery hardExtremely hard — deep expertise, careful design, and significant time required

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions