Repository navigation
Retest fixes: premature-download zip, SSE saturation, bad-zip 500, li… - #32
Merged
Merged
Conversation
…st/delete race External retest against latest main (523f369) confirmed the previous CORS + auth + LayerNorm fixes landed cleanly, and surfaced four new defects. This closes all four plus one nginx hygiene gap flagged in the report. P0-A (high) - Pending-merge download returned HTTP 200 + 22-byte empty zip, and the empty archive was then cached forever at _download.zip because `if not zip_path.exists()` reused the first build. Even after the merge completed, every subsequent download returned the same empty archive. Fix: reject non-complete downloads with 409 (`job is <status>; download only available for completed jobs`) AND invalidate any cached zip whose mtime predates job.finished_at, so a client that triggered the empty cache in a legacy build still gets a correct zip after redeploy. P0-D (high) - 16 concurrent /api/jobs/<id>/stream requests saturated all 16 gthread worker threads; health probes timed out with streams open. Fix: `_SSE_SEM = BoundedSemaphore(MAX_SSE_STREAMS)` capped at `max(threads - 2, 2)` by default, env-tunable via MERGESE_MAX_SSE_STREAMS. Non-blocking acquire at stream start: on exhaustion, 429 with Retry-After:5. The semaphore is released in `_tail_stream`'s finally so a client disconnect (generator closed by gunicorn) still frees the slot. P0-B (medium) - Posting non-zip bytes under 'file' returned HTTP 500 via the generic `except Exception` catch-all. Fix: catch `zipfile.BadZipFile` explicitly and return 400 with a diagnostic error ("re-create the archive, prefer `zip -r` over Finder's Compress which embeds AppleDouble metadata"). P0-C (medium) - Concurrent list/delete produced ~3 x 500 among 280 mixed operations. Tracebacks: FileNotFoundError in `d.stat().st_mtime` sort key and in the per-entry stat() during iteration. Fix: new helpers `_safe_iterdir` and `_safe_sorted_by_mtime` tolerate disappearing entries (mtime=0, drop at caller). Both listing endpoints (/api/uploads, /api/datasets) and the /api/library iteration over UPLOADS_ROOT + DATASET_UPLOADS_ROOT use them. Per-child `stat()` calls inside the body are wrapped in try/except FileNotFoundError. P1-E - nginx body caps: job-starting JSON routes (/api/{inspect,merge, evaluate,export}) still inherited the server-wide 3G default. The retest flagged that an attacker could stream 3G of JSON into gunicorn's memory before Flask would reject. Fix: `client_max_body_size 256k` on those four routes. 256k leaves headroom for a merge body with long preset lists; raise only if a real request hits it. /api/uploads + /api/datasets keep the 3G cap (binary uploads). Tests: 121 passed (was 117), 6 skipped. Four new regression tests: - test_malformed_zip_upload_returns_400 - test_list_tolerates_deleted_entries - test_sse_stream_cap_rejects_overflow_with_429 - test_download_refused_when_job_not_done Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…st/delete race
External retest against latest main (523f369) confirmed the previous CORS + auth + LayerNorm fixes landed cleanly, and surfaced four new defects. This closes all four plus one nginx hygiene gap flagged in the report.
P0-A (high) - Pending-merge download returned HTTP 200 + 22-byte empty zip, and the empty archive was then cached forever at _download.zip because
if not zip_path.exists()reused the first build. Even after the merge completed, every subsequent download returned the same empty archive.Fix: reject non-complete downloads with 409 (
job is <status>; download only available for completed jobs) AND invalidate any cached zip whosemtime predates job.finished_at, so a client that triggered the empty
cache in a legacy build still gets a correct zip after redeploy.
P0-D (high) - 16 concurrent /api/jobs//stream requests saturated all 16 gthread worker threads; health probes timed out with streams open.
Fix:
_SSE_SEM = BoundedSemaphore(MAX_SSE_STREAMS)capped atmax(threads - 2, 2)by default, env-tunable via MERGESE_MAX_SSE_STREAMS.Non-blocking acquire at stream start: on exhaustion, 429 with Retry-After:5.
The semaphore is released in
_tail_stream's finally so a clientdisconnect (generator closed by gunicorn) still frees the slot.
P0-B (medium) - Posting non-zip bytes under 'file' returned HTTP 500 via the generic
except Exceptioncatch-all.Fix: catch
zipfile.BadZipFileexplicitly and return 400 with adiagnostic error ("re-create the archive, prefer
zip -rover Finder'sCompress which embeds AppleDouble metadata").
P0-C (medium) - Concurrent list/delete produced ~3 x 500 among 280 mixed operations. Tracebacks: FileNotFoundError in
d.stat().st_mtimesort key and in the per-entry stat() during iteration.Fix: new helpers
_safe_iterdirand_safe_sorted_by_mtimetoleratedisappearing entries (mtime=0, drop at caller). Both listing endpoints
(/api/uploads, /api/datasets) and the /api/library iteration over
UPLOADS_ROOT + DATASET_UPLOADS_ROOT use them. Per-child
stat()callsinside the body are wrapped in try/except FileNotFoundError.
P1-E - nginx body caps: job-starting JSON routes (/api/{inspect,merge, evaluate,export}) still inherited the server-wide 3G default. The retest flagged that an attacker could stream 3G of JSON into gunicorn's memory before Flask would reject.
Fix:
client_max_body_size 256kon those four routes. 256k leavesheadroom for a merge body with long preset lists; raise only if a real
request hits it. /api/uploads + /api/datasets keep the 3G cap (binary
uploads).
Tests: 121 passed (was 117), 6 skipped. Four new regression tests:
Summary
Risk + rollback
Testing
pytestpasses locallypython server/app.pyand acurlagainst the changed endpoint.github/workflows/ci.ymlstill passesDeploy note (fill if the merge needs an operator action beyond
git pull && systemctl restart)