Cache adsb.lol responses and stop writing them back to the readsb file

[Purple10101]

Split out of #17 so it can be reviewed on its own merit rather than riding behind a one-line header fix. This is a data-integrity fix, not a refactor.

1. The proxy wrote adsb.lol data into readsb's own file

// Write to local file so tar1090's backend process can read it
await fs.writeFile(LOCAL_DATA_PATH, JSON.stringify(convertedData));

LOCAL_DATA_PATH is /run/readsb/aircraft.json — readsb's output file, which readsb rewrites at 1 Hz. Two writers, one file, no coordination.

Because getAircraftData() reads the local file first, the next request reads back the adsb.lol payload it just wrote and reports it as source: 'local'. Reproduced against the live API:

request 1  ->  X-Data-Source: adsb.lol   45 aircraft
               (local file now exists: 14450 bytes)
request 2  ->  X-Data-Source: local      45 aircraft   <-- same remote data
log:           "Falling back to adsb.lol (local file not found)..."
               "adsb.lol: 45 aircraft"
               "Local file: 45 aircraft"

X-Data-Source cannot be trusted to say where a fix came from, which matters given the wrong-region incidents we've chased.

The worse case is a node that actually has a receiver. When readsb momentarily reports zero aircraft — a normal gap — the proxy fetches adsb.lol and overwrites the real receiver's output with remote aircraft. Anything reading that file directly rather than through the proxy then sees another region's traffic as if the local receiver produced it.

Fixed by serving from an in-process cache. Nothing writes to the local file; readsb owns it exclusively.

2. One upstream fetch per HTTP request

Every request with an empty local file triggered a fresh fetch. receiver.json sets "refresh": 1000, so each open map polls at 1 Hz, plus adsb2dd on its own cadence.

Measured on jonathan-node-1:

	Upstream requests
Before	164 in 5 min = ~33/min
After (3 s TTL)	26 in 2 min = ~13/min

Roughly a 60% reduction. Worth noting that hammering a free community API at that rate from every node, with no User-Agent identifying who is doing it, is plausibly related to why adsb.lol introduced the contact-info rule in the first place — though I can't prove the connection.

3. Everything else

• Concurrent requests collapse onto one upstream fetch via a shared inFlight promise instead of each issuing its own.
• A failed refresh leaves the previous cache in place and never rejects, so a flaky upstream degrades into stale-but-served rather than an error.
• X-Data-Age-Ms / X-Data-Stale expose staleness, and payload.now is stamped at fetch time and never restamped on serve. blah2-api's extrapolation refuses to project more than 5 s, which is what the 3 s TTL is sized against.
• MAX_STALE_MS bounds how long a dead upstream keeps being served; past it the response reports source: 'none' rather than passing off an empty sky as a successful read.
• Non-200 responses call res.resume() so the socket is drained.
• /health reports cache age and cached aircraft count.

Note on provenance

This work was already present uncommitted in the working tree; I've committed it as-is without modification and reconstructed the rationale above from the code and from measurements on a live node. If the original author had other reasons, they aren't captured here.

Touches the same fetchUrl as #18, so whichever lands second needs a rebase. #18 is the urgent one.

🤖 Generated with Claude Code

[claude]

Review

Good fix — the core problem (adsb.lol data being written into readsb's own output file and read back as local) is real and the in-process cache is the right shape of solution. A few things worth looking at before merge.

Bug: failure-path retries aren't rate-limited by the TTL

cache.fetchedAt is only stamped on a successful fetch (server.js:117). The refresh gate in getAircraftData() compares against it:

const age = cache ? Date.now() - cache.fetchedAt : Infinity;
if (age >= CACHE_TTL_MS) {
  await refreshRemote();
}

If adsb.lol is down (or has never succeeded), cache stays null/stale forever, so age is Infinity on every request, and refreshRemote() fires on every request that isn't collapsed by a concurrent in-flight call — it's not throttled to once per CACHE_TTL_MS. During a sustained outage this reverts to hammering the upstream at the full client-poll rate, which is exactly the behavior the PR's "be a good citizen of a free API" motivation (no User-Agent, rate-limit risk) is trying to avoid — and it's precisely when adsb.lol is struggling that you'd most want backoff. Consider tracking a separate lastAttemptAt (set regardless of success/failure) and gating refreshRemote() on that instead of cache.fetchedAt.

Missing: no User-Agent on the upstream request

The PR description calls out that hammering adsb.lol "with no User-Agent identifying who is doing it" is plausibly why they added the contact-info rule — but fetchUrl/ADSBLOL_API still sends no User-Agent (or From/contact header). Since this PR is explicitly about being respectful to the upstream, it seems worth adding one here (or as a fast follow) rather than leaving it as a known gap.

Minor: refresh blocks the triggering request

When the cache expires, the request that trips age >= CACHE_TTL_MS awaits refreshRemote() inline (server.js:154), so that one request pays the full upstream latency (up to UPSTREAM_TIMEOUT_MS, 3s by default) instead of getting the stale-but-recent value immediately. A stale-while-revalidate approach (serve cache.payload immediately if present, and kick off refreshRemote() without awaiting it) would avoid the periodic latency spike without weakening the staleness guarantees you're already tracking via ageMs/stale. Not a correctness issue, just worth considering given the 1 Hz poll cadence this is tuned for.

Nit: removed try/catch around getAircraftData()

The old /data/aircraft.json handler wrapped the call in try/catch; the new one doesn't. It's currently safe because readLocalFile swallows its own errors and refreshRemote() never rejects (it has its own internal .catch), but that safety is an implicit invariant spread across two functions rather than something the handler enforces. Given http.createServer's async callback, an uncaught rejection here would surface as an unhandledRejection rather than a clean error response. Might be worth a one-line comment noting getAircraftData() is guaranteed not to throw/reject, so the next person touching it doesn't accidentally break that guarantee.

Docs: new env vars aren't in .env.example

ADSBLOL_CACHE_TTL_MS, ADSBLOL_TIMEOUT_MS, and ADSBLOL_MAX_STALE_MS all have sane defaults, but they're not mentioned in .env.example alongside the existing ADSBLOL_* vars, so an operator wouldn't discover them without reading the source.

Test coverage

No tests included, but there's no existing test harness in the repo (no package.json/test framework for proxy/), so that's consistent with current project conventions rather than a regression. Given this is explicitly framed as a data-integrity fix, even a small smoke test (e.g. spin up the server, hit /data/aircraft.json twice against a stubbed upstream, assert the local file is never written and the TTL is respected) would make the fix's core guarantee ("nothing writes to LOCAL_DATA_PATH anymore") regression-proof going forward — that's exactly the kind of invariant that's easy to accidentally reintroduce in a future edit.

What's solid

• res.resume() on non-200 upstream responses is a correct fix for the socket-drain issue.
• Collapsing concurrent requests onto one inFlight promise is correct — no race window, since the check-and-set on inFlight is synchronous (no await between them).
• payload.now being stamped at fetch time (not serve time) and never restamped is the right call given X-Data-Age-Ms and consumers doing their own extrapolation.
• MAX_STALE_MS honestly reporting source: 'none' instead of silently serving arbitrarily old data is a good defensive touch.

🤖 Generated with Claude Code

[Purple10101]

Rebased onto master after #18 merged.

Both changes touched fetchUrl, so it conflicted in two places. Resolved by keeping both sides:

• Constants — cache constants (CACHE_TTL_MS, UPSTREAM_TIMEOUT_MS, MAX_STALE_MS) and USER_AGENT from Send a User-Agent so adsb.lol stops returning 403 #18.
• fetchUrl — UPSTREAM_TIMEOUT_MS from this branch combined with the User-Agent header and body-reading error path from Send a User-Agent so adsb.lol stops returning 403 #18. Dropped this branch's res.resume(), since consuming the body to build the error message already drains the socket; noted that in a comment so it doesn't look like an oversight.

Cross-check: the resolved file is byte-identical to what has been running on jonathan-node-1 since yesterday, apart from that one added comment.

Re-tested against the live API after the rebase:

1. UA fix intact          X-Data-Source: adsb.lol, 45 aircraft, no 403
2. cache serving          second hit X-Data-Age-Ms: 1034, upstream fetches: 1
3. write-back removed     local file never created
4. no mislabelling        3 consecutive requests all report adsb.lol, never 'local'

Check 3 is the regression test for the bug this PR fixes — under the old code that request sequence created the local file and the second response came back labelled source: 'local'.

[claude]

Review

This is a solid fix for a real data-integrity bug — using an in-process cache instead of writing adsb.lol data into readsb's own output file is clearly the right call, and the reasoning in the PR description (reproduced against a live node) makes the "two writers, one file" problem easy to verify. A few notes below, nothing blocking.

Code quality

• refreshRemote() / inFlight correctly collapse concurrent requests onto a single upstream fetch, and the check-then-set on inFlight is safe since there's no await between the check and the assignment — no race despite multiple request handlers interleaving on the event loop.
• The never rejects contract on refreshRemote() is actually upheld: an exception thrown inside convertAdsbLolToReadsb (e.g. malformed upstream payload) would still land in the chained .catch, so getAircraftData() can safely drop its try/catch around the fetch path. Worth double-checking this stays true if that promise chain is ever refactored, since it's not enforced by anything explicit.
• Comments are appropriately used to explain why (TTL sizing vs. blah2-api's 5s extrapolation limit, timeout vs. consumer HTTP timeout, now being fetch-time not serve-time) rather than restating the code.

Potential bug (minor)

In getAircraftData(), when ADSBLOL_ENABLED is true, the local file exists but reports 0 aircraft, and there's no usable adsb.lol cache (e.g. cold start with upstream down), the final fallback:

return { data: localData || emptyPayload(), source: 'none', ageMs: 0, stale: false };

reports source: 'none' even though localData is real (just empty). Previously this case was reported as source: 'local'. The aircraft payload is identical either way (empty array), but since this PR is specifically about making X-Data-Source trustworthy, it's worth deciding intentionally whether "local file present but empty" should read as local or none — right now it silently changed.

Minor / non-blocking

• The three new env vars (ADSBLOL_CACHE_TTL_MS, ADSBLOL_TIMEOUT_MS, ADSBLOL_MAX_STALE_MS) aren't added to .env.example. ADSBLOL_USER_AGENT from Send a User-Agent so adsb.lol stops returning 403 #18 isn't documented there either, so this may just be existing convention, but since these three directly control cache/staleness behavior operators may want to tune, they seem worth surfacing.
• Blocking the request on await refreshRemote() when the cache is cold or expired means the first request after TTL expiry can take up to UPSTREAM_TIMEOUT_MS (3s default) to respond. That looks like a deliberate, documented tradeoff (kept under blah2-api's 5s timeout) rather than an oversight, just flagging it as a latency characteristic worth knowing about.

Test coverage

No tests are included, but the repo has no JS test framework/harness currently (no package.json, no test runner), so this isn't a regression in convention — just noting that this logic (TTL expiry, stale-serving, inFlight dedup, empty-vs-missing-local-file handling) would be a good candidate for unit tests if/when the project adds a test setup, given how easy the source-labeling edge cases are to get subtly wrong.

Security

No concerns — RECEIVER_LAT/LON/RADIUS come from trusted env config, not user input, and the User-Agent change is purely to satisfy adsb.lol's contact-info requirement.

Overall: correctness fix is sound and well-justified, the caching/dedup logic is careful, and the only real ask is to double check the local-empty-but-present labeling edge case above is the intended behavior.

[Purple10101]

Thanks — worked through all of it. Pushed 8bbf79a. Three addressed, three answered.

Fixed: failure-path retries weren't throttled

Confirmed, and it's the substantive one. cache.fetchedAt only advances on success, so while adsb.lol is failing age stays Infinity and every request starts its own fetch — inFlight collapses concurrent callers but not sequential ones.

Worth noting this wasn't hypothetical. Measured on jonathan-node-1 before anyone looked for this bug:

	Upstream rate
Through the 403 outage (failure path)	~33/min
Once succeeding (TTL working)	~14/min

So the cache halved upstream load when things worked and did nothing when they didn't — the opposite of the intent.

Fixed by tracking lastAttemptAt, set when an attempt begins regardless of outcome, and refreshing only when the payload has aged out and no attempt has been made in the same window.

Measured against the live API, 20 requests over 10 s with the upstream deliberately 403-ing (ADSBLOL_USER_AGENT set to a blocked token, which is a convenient way to make adsb.lol fail on demand):

before   20 upstream attempts   (one per request)
after     4 upstream attempts   (one per 3 s TTL)

Success path unchanged — 6 requests over 6 s produce 2 upstream fetches.

Fixed: .env.example

You hedged that this "may just be existing convention" — it isn't. .env.example already documents ADSBLOL_ENABLED and ADSBLOL_RADIUS, so the convention is to document ADSBLOL_* vars. Added all four, including ADSBLOL_USER_AGENT from #18.

Fixed: the never throws invariant

Added a comment at the call site recording that getAircraftData() is contracted never to throw, why that makes the missing try/catch safe, and that an unhandled rejection in an async createServer callback is worse than a bad response. Both reviews independently verified the invariant holds today; the comment is so the next edit doesn't silently break it.

Stale: "no User-Agent on the upstream request"

This one no longer applies. That review was posted at 08:29:39Z; the rebase bringing #18's User-Agent onto this branch landed at 08:30:26Z — 47 seconds later. USER_AGENT is defined at lines 26–27 and sent at line 50.

Pushing back: source: 'none' vs 'local' for an empty-but-present local file

it's worth deciding intentionally whether "local file present but empty" should read as local or none — right now it silently changed

It didn't change silently — there's an explicit comment on that return explaining the choice:

"No usable remote data. Report that honestly rather than passing off an empty sky as a successful read."

I'd keep 'none'. If adsb.lol is enabled and failing, we genuinely don't know whether the sky is empty or we simply have no data. Reporting 'local' there is exactly the kind of false confidence this PR exists to remove — and the payload is an empty array either way, so nothing downstream is misled by the honest label. Note the !ADSBLOL_ENABLED branch above still returns 'local', which is correct: with no fallback configured, an empty local file really is the local answer.

Deferring: stale-while-revalidate

Fair observation, and correctly identified in the second review as a deliberate tradeoff rather than an oversight. With a 3 s TTL and 1 Hz polling, roughly one request in three pays refresh latency, bounded by UPSTREAM_TIMEOUT_MS (3 s) and inside blah2-api's 5 s budget.

I'd rather not fold it in here: serving stale-then-revalidating changes the staleness semantics this PR just established, and it deserves its own change where the ageMs/stale behaviour can be reasoned about on its own. Happy to open a follow-up.

Acknowledged: test coverage

Agreed on both counts — no package.json or test runner exists in the repo, so this is convention rather than regression, and the write-back invariant is exactly the sort of thing that's easy to reintroduce. Worth a harness; out of scope for a fix that's currently the only thing standing between the fleet and no ground truth.

[claude]

Review

This is a well-targeted fix for a real data-integrity bug (two writers racing on readsb's own output file), and the second commit catches a genuine flaw introduced by the first (the refresh gate not throttling failed fetches) — good iteration, and both are backed by measurements against a live node rather than just reasoning about the code.

Strengths

• Moving from "write remote data into readsb's file" to an in-process cache is the right fix — readsb now owns LOCAL_DATA_PATH exclusively, and X-Data-Source can be trusted again.
• inFlight correctly collapses concurrent callers onto a single upstream fetch, and lastAttemptAt (added in the second commit) correctly throttles sequential retries too — the bug it fixes (unthrottled retries while upstream is down, i.e. exactly when you least want extra load) is a sharp catch.
• Failure handling degrades gracefully: a broken upstream leaves the previous cache in place and reports honestly via stale/source: 'none' rather than either throwing or silently passing off empty/wrong data as good.
• .env.example documents each new tunable along with why it's set to that value (e.g. TTL sized against blah2-api's 5s extrapolation budget) — genuinely useful for whoever tunes this on a real node later.

Things worth a look

1. Refresh latency lands on one unlucky request per TTL window. In getAircraftData(), the request that first observes cacheAge >= CACHE_TTL_MS && sinceAttempt >= CACHE_TTL_MS calls await refreshRemote() inline, so that request pays the full upstream latency (up to UPSTREAM_TIMEOUT_MS if adsb.lol hangs) before it gets a response. Other concurrent/near-concurrent requests skip the gate and serve the still-cached (if stale) payload instantly, so this is bounded and probably fine given MAX_STALE_MS already exists as a backstop — but if you want uniformly low latency for all callers, consider serving the current cache immediately and kicking off refreshRemote() without awaiting it (true stale-while-revalidate) when the TTL has expired. Not a blocker, just a tradeoff worth naming explicitly since it wasn't called out in the PR description.

2. Minor comment inaccuracy (proxy/server.js:60): "Consuming it also drains the socket, which res.resume() did before." res.resume() doesn't actually appear anywhere in this file, before or after this PR — master already drained non-200 bodies by consuming data/end (same as now). Harmless functionally, but worth fixing so the comment doesn't assert a history that didn't happen.

3. Silent misconfiguration for the new tunables. CACHE_TTL_MS, UPSTREAM_TIMEOUT_MS, and MAX_STALE_MS are all parseInt(process.env.X || 'default') with no validation. If one of these env vars is set to a non-numeric value, parseInt returns NaN, and since every comparison against NaN is false, cacheAge >= NaN is false — so refreshRemote() is never called and the adsb.lol fallback silently stops populating, with no log line indicating why. Low severity (matches the existing style for ADSBLOL_RADIUS/PORT elsewhere in the file), but three new tunables is a good point to consider a light guard, e.g. falling back to the default when the parsed value is NaN.

Test coverage

No test harness exists in this repo (it's install-script/shell-centric, no package.json), so there's nothing to add tests to here — that's consistent with the rest of the codebase. The commit messages document manual verification against the live API for both the caching behavior and the retry-throttle fix, which is good practice given the lack of automated coverage.

Security

Nothing concerning — this only changes caching/fallback logic for a public read-only aircraft feed, no new user input paths, and the User-Agent value is only ever sent outbound, never reflected.