Run a gene set analysis end to end: upload validation and the job - #291
Merged
Merged
Conversation
Second slice of spec 012. Two layers, neither importing Chainlit, because the awkward parts -- a job that outlasts a chat turn, a submission that succeeds and then fails, a result that must be three sizes for three audiences -- are the parts worth testing, and a browser is not needed to test them. **`upload.py`** caps a file at 20 MB against Chainlit's shipped 500 MB. The host has 4.7 GB free of 88 GB and `~/update-beta-chat.sh` refuses to deploy under 6 GB, so a handful of default-sized uploads would take the chat down *and* block the fix. Shape is checked here rather than by the service, which answers a malformed matrix minutes later through a status field, in R's words. **`job.py`** owns one analysis. `Finished` splits by audience: `for_model` bounded and allow-listed, `links` the capability URL the model must never see, `table_path` the full table for the user. Found while writing it: a one-sample file was told it was not an expression matrix -- it is one, it just cannot be compared against anything -- and `gene_count` returned `-1` for a file too long to finish counting, a sentinel of the same type as a real count, leaving a function whose result gets shown to someone. It is `None` now, and the type says so. **Adversarial review of the finished branch found four more, and three are the same mistake.** *The upload leaked on the likeliest paths.* The group checks sat above the `try`, so a wrong group name or a miscounted label list -- the two mistakes a user actually makes -- returned an error and left their matrix on the disk. The leak was on the paths people take most often. *The loading loop had only the wall-clock bound.* That is the bug I had fixed in `await_result` earlier in the same review, in the same file, twelve lines away. Fixing a loop is not fixing the loops. Both now carry `MAX_POLLS` as well. *`finished` and `failed` were defined on the status objects and used by nobody* -- the loops compared the strings themselves, so "done" had three definitions. One `TERMINAL_STATUSES` now, and `failed` is derived as "terminal and not complete", so a status added later is a failure by default rather than an analysis that silently reports no results. *Result tables were written and never removed.* The upload is deleted the moment it is submitted and then the output was kept forever, at ~2 MB each, on a host with 4.7 GB free. Deleting the input and hoarding the output is not a disk policy. `prune_results` runs before each write, by age then by total size, and only ever touches files this module named. **And the sabotage found a fifth.** Removing the `prune_results` call from `await_result` broke nothing: every pruning test called the function directly, so the directory could have grown forever with the suite green. That it works is not the same as it being called. Sabotage, each against the test written for it: validation back above the `try` fails both deletion tests; removing the prune call fails the new one; unbinding the loading loop hangs, which is the failure mode the bound converts into an error. 67 tests in tests/gsa, ./checks.sh clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamjohnwright
force-pushed
the
012-gsa-job
branch
from
September 22, 2026 01:52
17d8215 to
980ea94
Compare
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.
Second slice of spec 012. Two layers, neither importing Chainlit — the awkward parts (a job that outlasts a chat turn, a submission that succeeds and then fails, a result that must be three sizes for three audiences) are the parts worth testing, and a browser is not needed to test them.
upload.pycaps a file at 20 MB against Chainlit's shipped 500 MB. The host has 4.7 GB free of 88 GB and~/update-beta-chat.shrefuses to deploy under 6 GB, so a handful of default-sized uploads would take the chat down and block the fix. It checks shape before the service does, because ReactomeGSA answers a malformed matrix minutes later through a status field, in R's words.job.pyowns one analysis.Finishedsplits by audience:for_modelbounded and allow-listed,linksthe capability URL the model must never see,table_paththe full table for the user.Three findings from the review
Each from trying to break it, not from reading it.
A one-sample file was told it was not an expression matrix. It is one — it just cannot be compared against anything. The structural floor is now two columns, and "not enough samples" is its own refusal, so the message matches the mistake.
gene_countreturned-1for a file too long to finish counting: a sentinel of the same type as a real count, escaping a function whose result gets shown to someone — "your matrix has -1 genes". NowNone, and the type says so.The poll loop's only bound was wall-clock, and my first fix for that did not work. The deadline assumes every turn waits; with sleeping patched out, elapsed time never advances. I bounded iterations as
deadline / interval— which computes the bound from the quantity that is degenerate.poll_interval=0gives 1.8 million, so the guard against a runaway loop was unbounded in exactly the case it existed for.It left three pytest processes at 100% CPU on a shared host for twenty minutes before I noticed. There is now a flat
MAX_POLLSceiling as well; at the real interval a 30-minute deadline is 180 polls.Also pinned
dataset_name— it is user free text that would come back in the resultfinally, so a failed submission does not leave it on a full diskTumour, leftinto two samples is worse than refusing the fileVerification
test_a_failed_analysis_raises_rather_than_returning_nothingtest_an_uploaded_file_is_deleted_even_when_submission_failstest_refuses_a_file_over_the_capThe size-cap sabotage needed doing twice: the first attempt doubled an indent and was a syntax error, which proves nothing. A sabotage that does not compile is not a failing test.
59 tests in
tests/gsa,./checks.shclean.Still to come
The Chainlit glue:
AskFileMessage, the progress message, andcl.Filefor the results table.🤖 Generated with Claude Code
Adversarial review of the finished branch
Four findings, and three of them are the same mistake.
The upload leaked on the likeliest paths. The group checks sat above the
try, so a wrong group name or a miscounted label list — the two mistakes a user actually makes — returned an error and left their matrix on disk. The disk leak was on the paths people take most often, which is the worst place to have put it.The loading loop had only the wall-clock bound. That is the bug I fixed in
await_resultearlier in this same review, in the same file, twelve lines away. Fixing a loop is not fixing the loops. Both now carryMAX_POLLS.finishedandfailedwere defined and used by nobody. The loops compared the status strings themselves, so "done" had three definitions. There is oneTERMINAL_STATUSESnow, andfailedis derived as terminal and not complete — so a status the service adds later is treated as a failure by default, rather than as an analysis that quietly reports no results.Result tables were written and never removed. The upload is deleted the moment it is submitted, and then the output was kept forever at ~2 MB each, on a host with 4.7 GB free. Deleting the input and hoarding the output is not a disk policy.
prune_resultsruns before each write — by age, then oldest-first by total size — and only ever touches files this module named.The sabotage found a fifth
Removing the
prune_resultscall fromawait_resultbroke nothing. Every pruning test called the function directly, so the directory could have grown forever with the whole suite green. That a cleanup works is not the same as it being called, and only the path that promises the deletion can test the promise.Verification
tryprune_resultscalltest_writing_a_result_prunes_the_directory_firstfailsThe first attempt at the validation sabotage was a malformed indent and therefore a syntax error, which proves nothing; an
ast.parseguard now runs on every sabotage before the suite does.67 tests in
tests/gsa,./checks.shclean.