Fix/extend docker tar1090

[Sol455]

This PR patches a few issues with the tar1090 node container, mainly addressing the incorrectly configured lighttpd proxy in a container that actually runs nginx.

Key Changes

• Extend docker-tar1090 - single container with readsb built-in (removes redundant readsb container)
• Fix circular dependency - proxy reads from file (/run/readsb/aircraft.json) instead of HTTP, eliminating 5-7s timeouts
• Runtime nginx config injection - config persists after tar1090 regenerates it at startup
• Disable binCraft via nginx sub_filter for JSON compatibility with adsb.lol fallback
• Switch proxy port from 3000 to 3005 to avoid conflict with blah-2 api
• Update adsb fallback logic in server.js to switch to adsb.lol if 0 aircraft detected (previously readsb would return 0 aircraft instead of an error when misconfigured or missing a receiver, so the fallback was never triggered)

Why These Changes

The previous setup had a few issues worth addressing:

1. Dockerfile.tar1090 copied lighttpd config into an nginx-based container (which was ignored)
2. Proxy fetched from http://127.0.0.1:80/data/aircraft.json, creating a circular dependency and 5-7s timeouts
3. Separate readsb container was redundant since docker-tar1090 includes readsb
4. docker-readsb-protobuf has been archived (April 2025)

Files Changed

• Dockerfile - extend docker-tar1090, add Node.js proxy
• docker/01-start-proxy - s6-overlay cont-init script
• docker/08-inject-proxy-config - runtime nginx config injection
• proxy/server.js - file-based reading instead of HTTP fetch
• docker-compose.yml - simplified single container setup
• .github/workflows/docker_build.yml - use correct Dockerfile

Deleted (no longer needed)

• Dockerfile.tar1090 - replaced by Dockerfile
• docker/lighttpd-*.conf - we use nginx now
• docker/entrypoint.sh - replaced by s6-overlay scripts

Testing

Tested locally with adsb.lol. Would be good to test with an actual ADSB receiver (dev software stack containing these changes is retina-node v0.3.0-rc2, can be deployed via mender.

Discussion: Next Steps

This patch injects the nginx proxy and readsb config into the pre-built docker-tar1090 image. Going forward we could:

Option 1: Keep extending docker-tar1090 (current approach)

Pros: Easy updates from upstream, minimal maintenance
Cons: Have to inject our changes at runtime

Option 2: Fork docker-tar1090 from SDR Enthusiasts

Pros: Clean repo with only our files, can bake in proxy permanently
Cons: Maintenance burden, need to track upstream updates

Files that could be deleted (if staying with extend approach)

These are upstream tar1090 files we don't use since we're now using the pre-built sdr-enthusiasts image:

• 88-tar1090.conf, nginx.conf, nginx-readsb-api.conf (lighttpd/nginx configs)
• install.sh, uninstall.sh, tar1090.sh, tar1090.service (bare-metal install)
• cachebust.sh, cachebust.list, concat-example.sh, tag.sh (build scripts)
• toCSV.py, getupintheair.sh, version (utilities)
• html/ directory (docker-tar1090 has these)

[claude]

Pull Request Review: Fix/extend docker tar1090

Overview

This PR makes significant architectural improvements by consolidating the container setup, fixing the circular dependency issue, and switching from lighttpd to nginx.

✅ Strengths

• Excellent problem identification: The circular dependency causing 5-7s timeouts is a real issue
• Smart consolidation: Removing redundant readsb container simplifies setup significantly
• File-based reading: Eliminates circular dependency elegantly
• Runtime config injection: Using s6-overlay hooks numbered 08 is clever
• Good documentation: Inline comments explain the why
• Fallback logic improvement: Properly triggers adsb.lol fallback when 0 aircraft detected

🔴 Critical Issues

1. Race Condition in Process Management (docker/01-start-proxy:4)

Starting Node.js with & in a cont-init script creates an orphaned background process not managed by s6-overlay. If proxy crashes it won't restart.

Recommendation: Create proper s6 service at /etc/services.d/proxy/run with exec node /opt/proxy/server.js

2. Missing Error Handling in JSON Parsing (proxy/server.js:103)

If file contains malformed JSON this will throw unhandled exception crashing the proxy.

Recommendation: Wrap JSON.parse in try-catch block

3. Unsafe sed Command (docker/08-inject-proxy-config:8-9)

If pattern not found sed silently succeeds but doesn't inject config. Verification only checks if proxy_pass exists anywhere in file.

Recommendation: Validate injection anchor exists first and verify specific locations were added

⚠️ Important Issues

4. Missing timeout configuration on HTTP server - could lead to hanging connections
5. No environment variable validation for RECEIVER_LAT, RECEIVER_LON, ADSBLOL_RADIUS
6. Proxy port not exposed in docker-compose for debugging

🟡 Minor Issues

7. Inconsistent error logging patterns
8. Missing health check in docker-compose despite /health endpoint
9. Hardcoded timeout value could be configurable
10. Writing to same file read by readsb could have race conditions

🔒 Security

11. No input sanitization when converting adsb.lol data
12. CORS wide open with Access-Control-Allow-Origin: *

📊 Test Coverage

❌ No Tests Detected - Critical Gap for a change of this scope

Recommendations: Unit tests for proxy functions, integration tests, container tests for nginx injection, Docker Compose validation

🚀 Performance

13. File read on every request - consider short-lived cache if needed
14. Synchronous string building - could use Buffer.concat for large responses

📝 Documentation

15. Excellent PR description with clear problem statement and rationale
16. Missing upgrade path documentation for existing users

🎯 Summary

Must Fix Before Merge

1. Fix proxy process management - use s6 service
2. Add error handling for JSON parsing
3. Improve nginx config injection verification

Should Fix

4. Add environment variable validation
5. Add basic test coverage
6. Add timeout configuration to HTTP server

Overall Assessment

Verdict: Approve with required changes ✅⚠️

This is solid refactoring addressing real architectural problems. Core approach is sound but process management and error handling issues should be addressed before merging. Lack of tests is concerning for a change of this scope.

Estimated Risk: Medium-High
Recommendation: Fix critical issues 1-3, add basic tests, then merge

🤖 Review generated by Claude Code

[jonnyspicer]

Is my review blocking anything? I'll be afk for another ~1 week or so, if it's important I can find a way to take a look sooner though

[claude]

Pull Request Review: Fix/extend docker tar1090

Summary

This PR makes significant architectural improvements by addressing a circular dependency issue and consolidating the container setup. The changes are well-motivated and solve real problems with the previous implementation.

Positive Aspects

✅ Eliminates circular dependency - Reading from file instead of HTTP eliminates 5-7s timeouts
✅ Simplifies architecture - Single container approach removes redundant readsb service
✅ Well-documented - PR description and commit messages clearly explain the changes
✅ Runtime config injection - Smart use of s6-overlay numbering to persist nginx config
✅ Improved fallback logic - Now switches to adsb.lol when 0 aircraft detected

Issues & Recommendations

🔴 Critical: Proxy Service Management

Issue: docker/01-start-proxy:4 starts the proxy with & but has no process supervision.

node /opt/proxy/server.js &

Problem: If the Node.js process crashes, it won't restart. Using cont-init.d for a long-running service is not ideal with s6-overlay.

Recommendation: Use s6-overlay services properly:

1. Create /etc/services.d/proxy/run:

#!/command/with-contenv bash
exec node /opt/proxy/server.js

2. Remove docker/01-start-proxy or use it only for one-time initialization

This ensures s6 supervises the proxy and restarts it on failure.

---

🟡 Code Quality Issues

1. Error Handling in readLocalFile() (proxy/server.js:100-107)

async function readLocalFile() {
  try {
    const data = await fs.readFile(LOCAL_DATA_PATH, 'utf8');
    return JSON.parse(data);
  } catch (error) {
    return null;
  }
}

Issue: Silent error swallowing. File read errors vs JSON parse errors should be distinguishable.

Recommendation:

async function readLocalFile() {
  try {
    const data = await fs.readFile(LOCAL_DATA_PATH, 'utf8');
    return JSON.parse(data);
  } catch (error) {
    console.log(`Failed to read local file: ${error.message}`);
    return null;
  }
}

2. Race Condition (proxy/server.js:92)

await fs.writeFile(LOCAL_DATA_PATH, JSON.stringify(convertedData));

Issue: When using adsb.lol fallback, writing to /run/readsb/aircraft.json could conflict with readsb's own writes, causing corruption or partial reads.

Recommendation:

• Consider writing to a different file path for fallback data (e.g., /run/readsb/aircraft.fallback.json)
• Or add atomic write pattern with temp file + rename

3. Nginx Script Brittle sed Commands (docker/08-inject-proxy-config:8,13)

sed -i '/include \/etc\/nginx\/nginx-tar1090-webroot.conf;/i\  location = /data/aircraft.json { proxy_pass http://127.0.0.1:3005/data/aircraft.json; }' \
    /etc/nginx/sites-enabled/tar1090

Issues:

• Single long sed command is hard to read and maintain
• No validation that the target line exists before attempting insertion
• Spaces in the inserted config might not match nginx style

Recommendation:

# Check if target line exists
if ! grep -q "nginx-tar1090-webroot.conf" /etc/nginx/sites-enabled/tar1090; then
    echo "[08-inject-proxy-config] ERROR: Target line not found in nginx config"
    exit 1
fi

# Add proxy location with clearer formatting
sed -i '/include \/etc\/nginx\/nginx-tar1090-webroot.conf;/i\
  location = /data/aircraft.json {\
    proxy_pass http://127.0.0.1:3005/data/aircraft.json;\
  }' /etc/nginx/sites-enabled/tar1090

4. HTTP Request Cleanup (proxy/server.js:34-42)

req.on('timeout', () => {
  req.destroy();
  reject(new Error('Request timeout'));
});

req.on('error', reject);

Issue: Request is destroyed on timeout, but the error handler will also fire after destroy, causing double-rejection (though Promise only rejects once).

Recommendation:

req.on('timeout', () => {
  req.destroy();
  reject(new Error('Request timeout'));
});

req.on('error', (err) => {
  if (req.destroyed) return; // Ignore errors after timeout
  reject(err);
});

---

🔵 Best Practices & Improvements

5. Environment Variable Validation

The proxy server doesn't validate critical environment variables. If RECEIVER_LAT/RECEIVER_LON are invalid, the adsb.lol API URL will be malformed.

Recommendation: Add startup validation:

if (ADSBLOL_ENABLED) {
  if (isNaN(RECEIVER_LAT) || isNaN(RECEIVER_LON)) {
    console.error('ERROR: RECEIVER_LAT and RECEIVER_LON must be valid numbers when ADSBLOL_ENABLED=true');
    process.exit(1);
  }
}

6. Nginx Config Reload

After injecting nginx config, the config should be validated and nginx reloaded.

Recommendation: Add to docker/08-inject-proxy-config:

# Validate nginx config
if ! nginx -t 2>&1 | grep -q "successful"; then
    echo "[08-inject-proxy-config] ERROR: Invalid nginx configuration"
    exit 1
fi

# Note: No need to reload - s6-overlay will start nginx after this runs

7. Port Conflict Documentation

The PR mentions port 3005 was chosen to avoid conflict with "blah-2 api" on port 3000. This context should be in code comments.

Recommendation: Add to proxy/server.js:

// Default port 3005 to avoid conflict with blah-2 API on 3000
const PORT = parseInt(process.env.PROXY_PORT || '3005');

---

🟢 Performance Considerations

✅ File-based reading is much faster than HTTP (good decision)
⚠️ Consider: Add a file watcher to detect when readsb updates the file, rather than reading on every request. This could reduce disk I/O for high-traffic scenarios.

const fs = require('fs').promises;
const { watch } = require('fs');

let cachedData = null;
let cacheTimestamp = 0;

watch(LOCAL_DATA_PATH, async (eventType) => {
  if (eventType === 'change') {
    cachedData = await readLocalFile();
    cacheTimestamp = Date.now();
  }
});

---

🔒 Security Considerations

✅ No obvious security vulnerabilities
✅ Input sanitization not needed (data sources are trusted)
✅ No user input parsing
⚠️ Minor: The proxy listens on all interfaces (0.0.0.0 by default). Consider binding to 127.0.0.1 only since it's accessed via nginx:

server.listen(PORT, '127.0.0.1', () => {
  console.log(`Aircraft data proxy listening on 127.0.0.1:${PORT}`);
  // ...
});

---

📝 Test Coverage

❌ No tests found - This is a functional change affecting data flow

Recommendations:

1. Add unit tests for convertAdsbLolToReadsb() function
2. Add integration test that verifies:
   • Local file reading works
   • Fallback to adsb.lol when file missing
   • Fallback to adsb.lol when 0 aircraft
3. Test nginx config injection succeeds
4. Test proxy starts and responds to /health endpoint

---

📋 Documentation

✅ PR description is excellent
⚠️ Consider updating: README.md still references the old multi-container setup (sections about lighttpd, etc.)

---

Overall Assessment

This is a solid improvement to the codebase. The architectural decisions are sound and the implementation is mostly clean. The critical issue with proxy process supervision should be addressed before merging. The other issues are improvements that would increase robustness.

Merge Recommendation: ⚠️ Approve with Changes

Must Fix Before Merge:

1. Proper s6-overlay service setup for proxy (use /etc/services.d/)

Should Fix:
2. Race condition when writing to readsb's aircraft.json
3. Add error logging in readLocalFile()
4. Improve nginx sed command robustness

Nice to Have:
5. Environment variable validation
6. Security: Bind proxy to 127.0.0.1 only
7. Add test coverage
8. Update README for new architecture

---

Great work on identifying and solving the circular dependency issue! 🎉

[Sol455]

Is my review blocking anything? I'll be afk for another ~1 week or so, if it's important I can find a way to take a look sooner though

No rush!

[jonnyspicer]

A couple of small questions but looks great overall!

[claude]

Pull Request Review: Fix/extend docker tar1090

Summary

This PR successfully addresses critical architectural issues with the tar1090 Docker container setup. The changes eliminate a circular dependency, consolidate containers, and implement proper nginx configuration injection. Overall, this is a well-executed refactor with clear improvements to the system architecture.

Strengths

Architecture Improvements

• Excellent fix for circular dependency: Moving from HTTP fetch to file-based reading eliminates the 5-7s timeout issue elegantly
• Smart use of s6-overlay: Properly leveraging the base image init system with numbered scripts ensures correct execution order
• Container consolidation: Removing the redundant readsb container simplifies the architecture significantly

Code Quality

• Clear, well-documented code with good inline comments explaining the why behind technical decisions
• Proper error handling: The proxy gracefully falls back through multiple data sources
• Appropriate logging for debugging and monitoring

Issues and Recommendations

1. Process Management Issue (High Priority)

Location: docker/01-start-proxy:4

The proxy is started as a background process without proper process supervision. If it crashes, it will not restart.

Recommendation: Use s6-overlay service management by creating /etc/services.d/proxy/run instead of starting with ampersand in cont-init.d. This ensures the proxy is properly supervised and will be restarted if it crashes.

2. Fragile sed Commands (Medium Priority)

Location: docker/08-inject-proxy-config:8,13

Problems: The sed commands rely on finding an exact string that upstream could change. Complex quoting makes maintenance difficult. The verification only checks if proxy_pass exists anywhere in the file.

Recommendations: Add validation that the anchor line exists before attempting injection. Improve verification to check for the specific location blocks. Consider creating a separate nginx config snippet for more stability.

3. Missing Error Recovery (Medium Priority)

Location: proxy/server.js:92-95

When fetching from adsb.lol, if the write to LOCAL_DATA_PATH fails, it only logs a warning. However, this means tar1090 backend will not see the fallback data. Consider treating this as a more serious error.

4. Security: No Input Validation on Environment Variables

Location: proxy/server.js:7-10

Latitude and longitude values are parsed without validation. Malformed env vars could cause NaN values in the API URL. Add coordinate validation with proper min/max bounds (-90 to 90 for lat, -180 to 180 for lon).

5. Missing Healthcheck

Location: docker-compose.yml

The proxy implements a /health endpoint but there is no Docker healthcheck configured. Add a healthcheck directive to the compose file using curl or similar.

6. Port Configuration Inconsistency

Location: docker-compose.yml:31

The port is hardcoded as PROXY_PORT=3005 instead of using variable substitution like other variables. This prevents easy override.

7. Race Condition Risk (Low Priority)

Location: docker/01-start-proxy:4

The proxy starts in cont-init.d but nginx configuration injection happens in startup.d. There is a timing window where nginx has not been reconfigured yet. Documenting this dependency would help future maintainers.

8. JSON Parsing Without Validation (Low Priority)

Location: proxy/server.js:103

If the file contains valid JSON but in an unexpected format, it could cause issues downstream. Add basic schema validation to check that parsed data is an object.

Test Coverage

Critical Gap: No automated tests are present in the repository.

Recommendations:

1. Add integration tests for the proxy server (fallback logic, data format conversion, error handling)
2. Add smoke tests for the Docker container (verify nginx config injection, proxy responding, readsb integration)
3. Test the complete workflow (container startup with/without receiver, fallback activation, data format compatibility)

Security Assessment

Overall: Good

Positive findings:

• Eliminates HTTP circular dependency (reduces attack surface)
• File-based reading is safer than HTTP proxy
• Proper CORS headers
• No obvious injection vulnerabilities

Minor concerns:

• Lack of input validation on environment variables (addressed above)
• Consider rate limiting on the /data/aircraft.json endpoint if exposed publicly

Performance Considerations

Improvements:

• File reading is much faster than HTTP requests
• Eliminates 5-7s timeouts from circular dependency
• Synchronous file reads avoid connection overhead

Potential Optimizations:

1. Consider caching the local file read for a few seconds to avoid disk I/O on every request
2. The adsb.lol data is written to disk even when read from adsb.lol - this might cause unnecessary writes in fallback mode

Documentation

Good: PR description is comprehensive and well-structured. Inline comments explain complex logic. Clear rationale for changes.

Could be improved: Document the new environment variables (especially PROXY_PORT). Add troubleshooting guide for common issues. Consider adding migration notes for existing deployments.

Recommendations Priority

Must Fix (Before Merge):

1. Fix process management for the proxy (use s6-overlay services)
2. Add validation for nginx config injection

Should Fix (This PR or Next):

3. Add input validation for coordinates
4. Improve error handling for file writes in fallback mode
5. Add Docker healthcheck

Nice to Have (Future PRs):

6. Add automated tests
7. Consider caching for performance
8. Add comprehensive documentation

Final Verdict

Recommendation: Approve with Changes

This PR makes significant architectural improvements and solves real problems. The core changes are sound and well-implemented. The issues identified are mostly around robustness and edge cases rather than fundamental problems.

Suggested path forward:

1. Address the process management issue
2. Add nginx config validation
3. Merge and deploy for testing
4. Follow up with test coverage in a subsequent PR

Great work on identifying and fixing the circular dependency issue! The file-based approach is elegant and much more reliable.

[claude]

Pull Request Review: Fix/extend docker tar1090

Overview

This PR successfully addresses significant architectural issues with the tar1090 container setup by eliminating the circular dependency, consolidating containers, and switching from file-based to direct reading. The changes are well-motivated and represent a meaningful improvement.

---

Strengths

Architecture Improvements

• Excellent fix for circular dependency: Reading directly from /run/readsb/aircraft.json instead of making HTTP requests eliminates the 5-7s timeout issue
• Proper container consolidation: Removing the redundant readsb container makes sense given docker-tar1090 includes readsb
• Smart s6-overlay integration: Using numbered scripts (08-inject-proxy-config) to run after nginx config regeneration is a clever solution
• Good port conflict resolution: Moving from 3000 to 3005 avoids the blah-2 API conflict

Code Quality

• Clean, readable JavaScript with proper error handling
• Good separation of concerns (file reading, HTTP fallback, data conversion)
• Helpful logging statements throughout
• Well-commented shell scripts explaining the why

---

Issues and Concerns

1. CRITICAL: Process Management Issue (proxy/server.js + docker/01-start-proxy)

Severity: High

The proxy is started with node /opt/proxy/server.js & in docker/01-start-proxy:4, but there is no process supervision. If the Node.js process crashes, it will not restart.

Recommendation: Use s6-overlay service management instead of backgrounding. Create /etc/services.d/proxy/run instead of using cont-init.d. This ensures s6-overlay monitors and restarts the proxy if it crashes.

---

2. Security: Unvalidated sed Injection (docker/08-inject-proxy-config)

Severity: Medium

Lines 8 and 13 use sed -i to inject nginx config, but there is no validation that the target pattern exists before injection. If the upstream docker-tar1090 changes the nginx config format, the sed command will silently fail or inject in the wrong location.

Issues:

• If /etc/nginx/nginx-tar1090-webroot.conf does not exist, sed succeeds but does nothing
• Multiple runs could inject duplicate location blocks
• Line 17-21 only checks if proxy_pass exists anywhere, not if it was injected correctly

Recommendations: Add idempotency checks before injection, validate the target pattern exists, and consider running nginx -t to validate the config after injection.

---

3. Error Handling: Silent Failures (proxy/server.js)

Severity: Medium

Lines 91-95: Writing to LOCAL_DATA_PATH fails silently when fetching from adsb.lol. This means tar1090 backend will not get adsb.lol data if the write fails. Consider making this a hard error if it is critical for tar1090, or documenting why it is acceptable to fail silently.

Lines 100-106: readLocalFile() returns null for all errors (missing file, permission denied, corrupt JSON, etc.). Consider logging errors for debugging, except for ENOENT which is expected.

---

4. Race Condition: Proxy Start Timing (docker/01-start-proxy)

Severity: Low-Medium

The proxy starts in cont-init.d (runs during container initialization) but is backgrounded with &. There is no guarantee it is ready when nginx starts trying to proxy to it.

Impact: First few requests to /data/aircraft.json after container start might fail with 502 Bad Gateway.

Recommendation: Use s6-overlay service dependencies to ensure proper startup order, or add a readiness check in the nginx config with retry.

---

5. Type Safety: Unsafe parseInt (proxy/server.js:9-10)

Severity: Low

If ADSBLOL_RADIUS is set to a non-numeric string, parseInt() returns NaN, which will break the API URL.

Recommendation: Use parseInt with radix 10 and add fallback for NaN cases

---

6. Performance: No Caching (proxy/server.js)

Severity: Low

Every request to /data/aircraft.json reads from disk. For high-traffic scenarios, consider caching with a short TTL (1-2 seconds) to reduce disk I/O.

---

7. Docker Best Practices (Dockerfile:4)

Severity: Low

Installing npm pulls in many dependencies that are not needed at runtime (only nodejs is used). Consider installing Node.js without npm if no build step is needed, or using a multi-stage build.

---

Test Coverage

Status: No tests found

Recommendation: Add basic tests for:

1. convertAdsbLolToReadsb() function - verify data transformation
2. getAircraftData() fallback logic - verify local to adsb.lol fallback
3. HTTP endpoint responses for various scenarios

---

Security Considerations

1. No input validation on environment variables - malicious values could break the service
2. CORS set to wildcard (line 144) - acceptable for public data, but document this decision
3. No rate limiting - consider if this will be exposed to untrusted clients
4. HTTP proxy has no authentication - fine for internal use, but document the security boundary

---

Documentation

Strengths:

• Excellent PR description with clear motivation
• Good inline comments explaining complex logic
• Docker compose file has helpful comment sections

Missing:

• No README updates for the new single-container setup
• No migration guide for existing users
• No documentation of environment variables
• No troubleshooting section for common issues

---

Recommendations Before Merge

Must Fix:

1. Implement proper process supervision for the proxy (use s6-overlay services.d)
2. Add idempotency checks to sed commands (prevent duplicate injections)
3. Validate nginx config format before injection (fail fast on upstream changes)

Should Fix:

4. Add basic error logging in readLocalFile()
5. Fix parseInt to handle NaN cases
6. Add nginx config validation (nginx -t)

Nice to Have:

7. Add basic tests for data transformation logic
8. Update README with environment variables and deployment instructions
9. Add startup readiness checks to prevent race conditions
10. Consider caching for high-traffic scenarios

---

Final Verdict

Recommendation: Approve with changes

This PR represents a significant architectural improvement that addresses real production issues. The core changes are well-designed and properly motivated. However, the process management issue should be addressed before merge to ensure reliability in production.

The sed injection concerns are less critical but should be addressed to prevent future breakage when docker-tar1090 updates.

Great work on identifying and fixing the circular dependency issue!

[claude]

Pull Request Review: Fix/extend docker tar1090

I've completed a thorough review of this PR. Overall, this is a well-architected solution that addresses significant architectural issues in the previous implementation. The changes are logical and well-documented.

---

✅ Strengths

Architecture & Design

• Excellent solution to circular dependency: Moving from HTTP fetch to file-based reading eliminates the 5-7s timeout issue elegantly
• Smart use of s6-overlay: Properly integrates with the base image's init system using numbered scripts to ensure correct execution order
• Good separation of concerns: Runtime nginx injection correctly handles the fact that tar1090 regenerates config at startup
• Simplified deployment: Consolidating from 2 containers to 1 reduces complexity since docker-tar1090 already includes readsb

Code Quality

• Well-documented changes: Inline comments clearly explain why decisions were made
• Proper error handling: The nginx injection script verifies its changes with a grep check
• Graceful degradation: Returns empty aircraft array instead of error when no data sources available - good UX decision

---

⚠️ Issues & Concerns

1. Process Management - Critical Issue ⚠️

Location: docker/01-start-proxy:4

Problem: Starting Node.js with a simple background operator means no process supervision, no logging integration with s6-overlay, and no graceful shutdown handling.

Recommendation: Convert this to a proper s6 service by creating /etc/services.d/proxy/run with exec node /opt/proxy/server.js. This ensures automatic restarts, proper logging, and coordinated shutdown.

2. Race Condition Risk ⚠️

Location: docker/01-start-proxy

Problem: The proxy starts in cont-init.d during container initialization, but there's no guarantee that /run/readsb/aircraft.json exists or that readsb is ready.

Recommendation: Add a wait loop or health check, or accept that initial requests may fail until readsb is ready.

3. Security: Command Injection Vulnerability 🔒

Location: docker/08-inject-proxy-config:8-14

Risk Level: Low (currently no user input in these sed commands)

Recommendation: This is fine for now, but if you ever parameterize these commands, use proper escaping or heredocs.

4. Error Handling in server.js

Location: proxy/server.js:100-106

Problem: readLocalFile() catches all errors and returns null, hiding different failure modes like permission denied or malformed JSON.

Recommendation: Consider logging different error types for debugging purposes.

5. Missing File System Synchronization

Location: proxy/server.js:92

Problem: Writing to /run/readsb/aircraft.json without synchronization could cause race conditions, partial writes, or file corruption if both tar1090's readsb and the proxy write to this file.

Question: Does tar1090's readsb actually write to this file, or only read from it? Clarify the ownership model.

Recommendation: If the proxy should only read, remove the write logic. If both can write, implement file locking or atomic writes.

6. Port Configuration Inconsistency

Location: docker-compose.yml:31

Observation: PROXY_PORT=3005 is hardcoded in docker-compose but has a fallback in server.js.

Recommendation: Document why 3005 was chosen in a comment or make it consistently configurable.

---

🧪 Testing & Coverage

No Tests Found ❌

Critical Gap: No test files exist in the repository.

Recommended Test Coverage:

1. Unit tests for server.js (transformation, fallback logic, error handling)
2. Integration tests (container startup, nginx config injection)
3. End-to-end tests (full docker-compose startup, data flow)

Suggestion: At minimum, add smoke tests that verify container starts successfully, proxy listens on port 3005, nginx config injection succeeds, and health endpoint returns 200.

---

🔒 Security Assessment

Low Risk Issues:

1. No input validation: The proxy trusts all data from adsb.lol
2. No rate limiting: Unlimited requests to adsb.lol fallback
3. CORS wide open: Access-Control-Allow-Origin allows any origin (probably acceptable for public aviation data)

Good Security Practices:

✅ No secrets in code
✅ Minimal container attack surface
✅ Read-only file operations (mostly)

---

📊 Performance Considerations

Positive:

✅ Eliminates 5-7s timeout - Major improvement
✅ File-based reading - Much faster than HTTP
✅ No unnecessary layers - Direct file access

Potential Concerns:

1. No caching: Every request reads from disk or hits adsb.lol (probably negligible for low-traffic)
2. Synchronous file writes: Could be an issue if fallback is used frequently
3. JSON parsing on every request: Fine for current scale

---

📝 Documentation & Maintainability

Excellent:

✅ PR description is comprehensive
✅ Inline comments are helpful
✅ Commit messages follow convention

Could Improve:

1. No inline documentation of the architecture - Consider adding ARCHITECTURE.md
2. Missing environment variable documentation
3. No upgrade/migration guide for users upgrading from previous version

---

🎯 Best Practices

✅ Following Best Practices:

• Extends base image rather than forking
• Uses s6-overlay properly
• Cleans up apt cache in Dockerfile
• Proper shebang for s6 scripts

❌ Not Following Best Practices:

1. No health check in Dockerfile: Add a HEALTHCHECK instruction
2. npm installed but not used: Line 4 of Dockerfile installs npm but it's never used - consider removing it

---

🚀 Suggestions for Future Improvements

1. Metrics/Monitoring: Add Prometheus metrics for request counts, fallback failures, and response times
2. Configuration validation: Check required env vars at startup
3. Structured logging: Use JSON logging for easier parsing
4. Graceful degradation testing: Simulate readsb failures in a test environment

---

Summary

Category	Rating	Notes
Architecture	⭐⭐⭐⭐⭐	Excellent solution to circular dependency
Code Quality	⭐⭐⭐⭐	Clean, well-documented code
Security	⭐⭐⭐⭐	No major concerns, minor hardening possible
Testing	⭐	Critical gap - no tests
Performance	⭐⭐⭐⭐	Significant improvement over previous version
Documentation	⭐⭐⭐⭐	Good PR docs, could use more architecture docs

Verdict: Approve with Recommendations

This PR solves real problems and is well-executed. The critical issues (process management, race conditions) should be addressed, but they don't block merging since they're improvements over the previous implementation. The lack of tests is the biggest concern for long-term maintainability.

Priority Actions:

1. High: Convert proxy to s6 service (process supervision)
2. Medium: Add basic smoke tests
3. Medium: Remove unused npm from Dockerfile
4. Low: Improve error logging in readLocalFile()
5. Low: Add HEALTHCHECK to Dockerfile

Great work on this refactor! 🚀

[claude]

Pull Request Review: Fix/extend docker tar1090

Summary

This PR makes significant architectural improvements to fix circular dependencies and simplify the container setup. The changes are well-motivated and address real issues. Overall, this is a solid refactor with good technical decisions.

✅ Strengths

Architecture & Design

• Eliminates circular dependency: Reading from /run/readsb/aircraft.json file instead of HTTP is a smart fix for the 5-7s timeout issue
• Simplified container setup: Single container approach is cleaner than the previous two-container setup, especially since docker-readsb-protobuf is archived
• Runtime config injection: Using s6-overlay startup scripts (numbered 08 to run after tar1090s 07) is the correct approach for persisting nginx config
• Improved fallback logic: Detecting 0 aircraft and switching to adsb.lol is more robust than the previous error-only approach

Code Quality

• Good use of s6-overlay conventions (/etc/s6-overlay/startup.d/ and /etc/services.d/)
• Clear comments explaining why runtime injection is needed
• Proper error handling in the proxy server
• Graceful degradation when data sources are unavailable

⚠️ Issues & Concerns

🔴 High Priority

1. Fragile nginx config injection (docker/08-inject-proxy-config:8,13)

The sed commands rely on finding the exact string include /etc/nginx/nginx-tar1090-webroot.conf;. If the upstream image changes this line, the injection will silently fail to insert at the right location (though it will catch the failure to inject via the grep check).

Recommendation: Consider adding a check that the target line exists before attempting injection.

2. Security: Complex quoting in sub_filter (docker/08-inject-proxy-config:13)

The complex quoting in the sub_filter sed command is hard to audit and potentially fragile. While not currently exploitable (no user input), it is unnecessarily complex. Consider using a heredoc or separate config file for better readability.

3. Race condition: File read without retry (proxy/server.js:100-107,110-116)

The proxy reads /run/readsb/aircraft.json once per request. If readsb is writing to the file when Node.js reads it, you may get incomplete JSON or read errors. This is a classic TOCTOU issue. Consider adding retry logic with a small delay (3 retries with 10ms between attempts).

🟡 Medium Priority

4. Missing input validation (proxy/server.js:7-10)

Environment variables are parsed but not validated. Invalid values could cause issues: negative radius, NaN for coordinates, port conflicts. Add validation with early exit on invalid config.

5. No rate limiting for adsb.lol API (proxy/server.js:84-98)

Every request that needs fallback will hit the adsb.lol API. Under heavy load, this could trigger rate limits. Consider adding caching with a 5-10 second TTL.

6. Inconsistent error responses (proxy/server.js:148-160)

On error, the server returns HTTP 200 with empty aircraft array. This masks failures and makes debugging harder. While this may be intentional for graceful degradation, consider using 503 Service Unavailable when no data source is available, or at least log more details.

7. Missing health check validation (proxy/server.js:161-163)

The /health endpoint always returns 200, even if the proxy cannot read local data or reach adsb.lol. Consider checking if at least one data source is accessible.

🟢 Low Priority

8. Port conflict documentation (docker-compose.yml:31)

The PR description mentions switching from port 3000 to 3005 to avoid conflict with "blah-2 api" but this is not documented in the compose file. Add a comment explaining this.

9. Missing Node.js version specification (Dockerfile:4)

Installing nodejs npm will use the distros default version (likely old). Consider specifying a version or using NodeSource for a modern Node.js version.

10. No graceful shutdown (proxy/server.js)

The Node.js server does not handle SIGTERM/SIGINT. While s6-overlay will kill it eventually, graceful shutdown is better practice.

🧪 Test Coverage

Missing: This PR has no automated tests. Consider adding:

1. Unit tests for convertAdsbLolToReadsb() data transformation
2. Integration test that validates nginx config injection
3. Mock test for fallback behavior

📊 Performance

The file-based approach is more efficient than HTTP (no network stack overhead). However, consider:

• Monitoring file read performance under high request load
• The adsb.lol API calls are blocking; consider a background refresh pattern if fallback is commonly used

🔒 Security

• ✅ No obvious security vulnerabilities
• ✅ CORS headers are appropriately permissive for public aircraft data
• ✅ No sensitive data in environment variables
• ⚠️ Consider adding request size limits to prevent memory exhaustion
• ⚠️ The nginx config injection could be more robust (see issue Get tar1090 working on blah2 raspberry pi node #1)

📝 Documentation

• ✅ Good inline comments in the Dockerfile
• ✅ PR description is thorough with clear motivation
• ⚠️ Missing: How to test locally without actual ADS-B receiver
• ⚠️ Missing: Environment variable documentation in README

🚀 Deployment Considerations

• ✅ Simplified docker-compose.yml is easier to maintain
• ✅ GitHub Actions workflow updated correctly
• ⚠️ Users upgrading from the old two-container setup may need migration guidance
• ⚠️ Consider documenting the new volume requirements (though tmpfs is already used)

Verdict

Recommend: Merge with minor fixes

This is a solid improvement that fixes real problems. The architectural decisions are sound. Address the high-priority issues (especially the race condition and nginx injection robustness), and this will be production-ready.

The code is well-structured and the approach of extending the upstream image rather than forking is pragmatic. Good work! 🎯

---

Reviewed with Claude Code

[Sol455]

A couple of small questions but looks great overall!

Great, thank you for looking over!

[jonnyspicer]

Looks great, ship iiiiiiiittttt 😁