Give every run its own directory, and stop moving the process - #46
Merged
Merged
Conversation
jorgeMFS
force-pushed
the
per-run-workspace
branch
from
August 16, 2026 14:28
7444be0 to
1b6abec
Compare
jorgeMFS
force-pushed
the
refuse-unsafe-upload-names
branch
from
August 20, 2026 10:42
ec5fc71 to
672e51e
Compare
jorgeMFS
force-pushed
the
per-run-workspace
branch
from
August 20, 2026 10:42
16fdc10 to
e306777
Compare
…the process Recorded before changing anything, so #40 rests on measurement. The handler moves the whole process: os.chdir("tmp") at main.py:29, and back at main.py:65 -- on the last line of the happy path, not in a finally. os.chdir is process-global and this is an async server, so the current directory belongs to everything in flight at once. Deterministic and tested here: a request with a malformed body 500s and leaves the process inside tmp/ the next request then chdirs relative to that and creates tmp/tmp/ a run writes to the constant "tmp", with nothing naming the run The interleaving case is not made into a test because it needs either an upload over starlette's 1 MiB spool threshold or more than one worker, and a test that depends on a timing window is worth less than the measurement. Measured separately: under one worker with parts below 1 MiB the handler has no await between the two chdir calls -- UploadFile.read is "if self._in_memory: return self.file.read(size)" -- so requests serialise and cannot interleave. Above the threshold the part spools and the read becomes a real await; with several workers it happens at any size, and one run's snakemake reads the Snakefile another run wrote. Also measured, because the fix depends on it. Replacing os.system with a timeout is not enough on its own: the naive form, which is what subprocess.run(timeout=N) does internally, kills snakemake but leaves the tool alive and then blocks forever on pipes the orphan still holds. naive communicate STILL BLOCKED after 7.0s, 1 orphaned process process grp communicate returned after 2.0s, 0 orphaned processes start_new_session=True plus os.killpg on a pgid captured before the child can be reaped is the difference between a killed step and a hung request. Refs #40, #11
os.chdir is gone. snakemake takes -s and -d, so the Snakefile and the working
directory are passed explicitly: relative paths in the rules resolve under -d
and the shell blocks run with that as their cwd, which leaves the emitter's
./{bin} convention untouched. Nothing needs the process to be somewhere.
The consequence is that the cleanup in the finally removes a directory instead
of restoring global state, so the failure recorded in the previous commit -- a
failed request stranding the process in tmp/, the next one nesting tmp/tmp/ --
no longer has anything to act on.
Containment is structural rather than a check. Every path is opened relative to
a descriptor held on the workspace for the run's lifetime, with O_NOFOLLOW and
O_CLOEXEC, and check_name guarantees a single component so there is no "/" for
a path to escape through. The descriptor is opened once: re-resolving the
directory each time would reintroduce the race it exists to remove. Measured on
this branch: 20000 iterations of a thread swapping a slot for a symlink to an
outside file, 0 escapes.
That descriptor also closes #41 on the read path. Outputs were read with a bare
open() and base64ed into the response, so a tool that replaced its own output
with a symlink had the target returned to the client with HTTP 200. open_read
refuses it.
Uploads are exclusive (O_EXCL) and the tools are placed first, so an upload
named after a binary is an error rather than a silent replacement of the thing
about to be executed. Note O_EXCL fires before O_NOFOLLOW, so a symlinked slot
is refused on write as FileExistsError and on read as ELOOP; both are mapped.
The tool cache is split out because per-run directories force it, not to tidy
up. fetch_tool memoises and returns early on a hit, so it never re-copied the
binary -- with one shared directory that made the copy persist between runs,
and with a fresh directory per run it would simply be absent on the second.
Bundles are now pulled once into BIOCHEF_TOOL_CACHE, staged as .part and moved
with os.replace so an interrupted pull cannot be read as complete, and
materialise_tools puts a copy into whichever workspace needs it. fetch_tool
keeps its two-argument signature, so the parse never learns about workspaces.
os.system is replaced by Popen with a timeout. The timeout kills the process
GROUP: killing only snakemake leaves the tool running and then blocks forever
on the pipes the orphan holds, measured at 7s and counting against 2s for the
group. The pgid is captured before the first wait, because getpgid raises once
the child is reaped. The whole thing runs in a threadpool, since communicate
blocks and a service that cannot have two runs in flight cannot have the
problem this PR is about.
Configuration, all defaulting to the safe value: BIOCHEF_RUN_ROOT,
BIOCHEF_RUN_TIMEOUT (finite), BIOCHEF_KEEP_WORKSPACE (off), BIOCHEF_TOOL_CACHE.
These change where and how long, never whether.
Verified end to end with a real snakemake and a real tool, not only in parts:
an upload lands, the tool is materialised, the workflow runs, the output comes
back base64ed, and the workspace is gone afterwards.
Closes #40, #41
materialise_tools copied a binary for every node, so two nodes naming the same executable made the second fail O_EXCL with FileExistsError and the request returned 500. This is not a corner case. 80 of the 176 catalogue operations share an executable with another: seqtk 20 operations bcftools 12 ivar 6 fastp 3 samtools 15 operations bedtools 12 muscle 4 gffread 4 So "samtools sort" feeding "samtools index" -- about the most ordinary pipeline there is -- returned 500 on this branch and 200 on its base. A regression, not a pre-existing gap. The old fetch_tool hid it behind its memo: on a cache hit it returned early and skipped the copy entirely, so the second node was a no-op. Splitting the cache out of the run directory removed the memo's side effect and nothing replaced it. No test in the original change used more than one tool node, which is why it got through. There is one now. Refs #40
Three findings from an adversarial pass, all in this PR. An upload named "Snakefile" occupies the slot the generated write needs, so O_EXCL refused it and the request died with an unhandled 500. The refusal was right -- run_snakemake is never reached and the attacker's file is never executed -- but the status was wrong for what is a bad request. Mapped to 400, like the upload loop. Also covered for "SNAKEFILE" and "snakefile", because APFS is case-insensitive by default and they occupy the same slot. test_a_timeout_kills_the_whole_process_group was a tautology. It re-implemented Popen, getpgid and killpg inline and asserted about its own local copy, so it passed against run_snakemake gutted to `return 0, "", ""`, and did not notice os.killpg being replaced by process.kill(). The argument for group-killing sits in the docstring right above it, so a reader would have taken the test as evidence for a property it never touched. It now drives main.run_snakemake, on a thread with a bounded join because the failure being guarded against is a hang. O_NOFOLLOW had no test at all. Dropping it left the suite green, even though it is what closes #41 -- outputs were read with a bare open() and base64ed into the response, so a symlinked output returned the target's contents with HTTP 200. Two tests now: the read path, and a non-exclusive write, which is the only way to observe the flag at all since O_EXCL otherwise refuses first. Each of the three was checked by making the mutation and confirming the suite fails: killpg -> process.kill() 1 failed drop O_NOFOLLOW 2 failed unmap the Snakefile FileExistsError 3 failed drop start_new_session the runner's own process group is killed -- caught, violently Writing the last of those found one more bug, in the test rather than the code: patching main.subprocess.Popen patches the subprocess module globally, so subprocess.run(["kill", ...]) later in the same test launched the slow helper instead of kill. It reported a surviving grandchild that it had just started itself and left processes running until the suite took 304 seconds. The substitution is now scoped to the snakemake argv, and the liveness check uses os.kill directly. Refs #40, #41
…ered Two gaps found by auditing coverage rather than reading the diff. fetch_tool's staging logic is code this branch adds, and no line of it ran: every existing test monkeypatches fetch_tool away or pre-populates TOOL_CACHE by hand. Covered now with a fake registry that records what was pulled and where -- that the pull is staged as .part and moved with os.replace, that a leftover .part from an interrupted pull is discarded rather than mixed in, that a bundle already on disk is not pulled again after a restart, and that a tool id which is not a plain name is refused. The branch turning a non-zero snakemake exit into a 500 had never been taken. Replacing `if code != 0:` with `if False:` left the suite green at 36 passed. A tool exiting non-zero is the ordinary outcome of a real run, so both the status and the shape of the body are now pinned, including that stderr is echoed back -- which is a deliberate choice and should fail a test if someone changes it without meaning to. Its sibling test asserts the workspace is still removed on that path, since the finally has to run there too and only the parse-error path was covering it. Mutation-checked, four caught: if code != 0 -> if False 1 failed skip the rmtree before staging 1 failed write in place instead of .part 5 failed no cleanup on failure 3 failed One mutation survives and is left alone: removing the in-memory memo from fetch_tool changes nothing observable, because the on-disk bundle.json check already prevents a second pull. It is an equivalent mutant, not a gap -- the disk path has its own test. Refs #40, #9
O_NOFOLLOW closes the symlink form of #41 and not the hard link form, because a hard link is not a symlink -- it is another name for the same inode and indistinguishable from the original. A tool could link a file from outside the run into its own output slot, and the agent would read it and base64 it into the response. Verified before the fix, on this branch: a tool hardlinks an outside file into its own output slot open_read returns it: b'PATIENT_ID,GENOTYPE\nNA12878,0/1\n' A file this workspace created has exactly one link; anything with more was linked from elsewhere. st_nlink is 2 for a hardlinked slot and 1 for a genuine output, so the check is one fstat. I had this reported and dismissed it, on the grounds that it needs a process already executing on the host, at the same uid, and therefore crosses no boundary. That reasoning came from the wrong threat model. The tool binary IS the untrusted party -- arbitrary code pulled from a registry, running against whatever the deployment can see -- so "the attacker already runs code on the host" describes the ordinary situation rather than an escalation. Where the agent sits next to data that may not leave, the response body is the way out, and that is the deployment this service is for. Mutation checked: weakening the comparison fails the new test. The companion test asserts an ordinary output is still readable, so the check cannot pass by refusing everything. Refs #41, #40
…rkspace Two more consequences of correcting the threat model. Both were reported, and I dismissed both on the grounds that they need code already running on the host or that only the caller is affected. Neither holds where the tool is the untrusted party and the deployment sits next to data that may not leave. An upload could occupy a slot the run means to produce. snakemake then sees the output already present and up to date, skips the rule that would have made it, and the client's bytes come back as that tool's output -- with nothing in the response saying the tool never ran. Measured before the fix, through the real handler: uploading "t-1-out" alongside the real input returned 200 with "FORGED-BY-CLIENT" where the genuine run returns "GENUINE-TOOL-OUTPUT". I had called this a correctness wart on the grounds that the only party deceived is the caller about its own run. That is not true once E5 provenance and F7 audit exist: the record is what a site custodian relies on. O_EXCL cannot catch it, because at upload time the output does not exist yet. What catches it is the workflow itself. Every intermediate file is named for the edge that carries it, so what a run consumes and what it produces are both known before anything runs, and what is consumed but not produced is exactly what has to arrive as an upload. This is the second of the two gates workspace.py already described and this branch had left unimplemented. It subsumes two earlier refusals. An upload named "Snakefile" is now refused as "not an input" before anything is written, rather than by O_EXCL when the generated write contends for the slot. Reaching the exclusive-open refusal now requires sending a legitimate input twice, which is what its test does. The same gate answers the other half: a declared input that never arrives is refused by name, instead of the run proceeding and failing inside snakemake looking for a file nobody sent. Second, cleanup deleted by path. rmtree takes a path, and a path is a lookup rather than a handle, so a directory moved or replaced after it was opened would have something else deleted in its place. The descriptor is held precisely so identity does not depend on the path; cleanup now compares the inode the path resolves to against the one opened at construction and declines if they differ. Leaking a directory is the right failure; deleting the wrong one is not. Mutation checked, all four now caught: declared-set gate removed 3 failed missing-input check removed 1 failed cleanup identity check removed 1 failed identity always matches 1 failed 48 tests pass. Refs #40, #41, #39
jorgeMFS
force-pushed
the
refuse-unsafe-upload-names
branch
from
August 20, 2026 11:41
672e51e to
0be3995
Compare
jorgeMFS
force-pushed
the
per-run-workspace
branch
from
August 20, 2026 11:41
e306777 to
c0c229c
Compare
This was referenced Aug 20, 2026
It failed 10 times in 25 runs under CPU contention, and never on the behaviour it names. Every failure was "the helper never spawned its grandchild". The test set timeout_s=1, but the clock starts at Popen and the grandchild does not exist until /bin/sh has been forked, has backgrounded sleep, and has written the pid. When the kill won that race there was no pid to read, so the test failed on its own precondition. Time to readiness, measured over 60 launches under six spinning cores: median 435 ms, p95 602 ms, max 695 ms. A one second budget left a couple of hundred milliseconds of margin and the suite's own contention was enough to spend it. Readiness is now waited for and asserted separately, so a helper that never started reports exactly that instead of being dressed up as a failure to kill the process group -- the two are not the same finding, and only one of them is about the code. The budget is 5s, which is roughly seven times the worst measured startup. The cost is honest: testing a timeout takes about as long as the timeout, so this test now runs for five seconds rather than one. Measured after the change, same six-spinner load: before 10 failures / 25 runs after 0 failures / 40 runs, and 0 / 30 under eight spinners One failure in a separate 25-run batch was not captured, so this is a large reduction rather than a proof of zero. Still catches what it exists for -- with os.killpg(pgid, SIGKILL) replaced by process.kill(), the grandchild is orphaned and the test fails.
Contributor
Author
|
Pushed a fix for a flaky test on this branch, found while auditing #57.
It set Measured time to readiness, 60 launches under six spinning cores:
A one-second budget left a couple of hundred milliseconds of margin, and the suite's own contention was enough to spend it. Readiness is now waited for and asserted separately, so a helper that never started reports exactly that instead of being dressed up as a failure to kill the process group — those are different findings and only one is about the code. Budget raised to 5 s, roughly seven times the worst measured startup. The cost is honest: testing a timeout takes about as long as the timeout, so this test now runs five seconds instead of one. One failure in a separate 25-run batch was not captured, so this is a large reduction rather than a proof of zero. Still catches what it exists for — replacing Worth knowing: this was live on master-bound work, so any green CI run on this stack had roughly a 1-in-6 chance of a spurious failure. |
Two problems with the previous fix, found by soaking it. The readiness wait was vacuous. It waited for the pid file to appear and asserted only that it existed, which is satisfied by an empty file, a stale number, or a grandchild that has already exited -- in each case the test then asserts about a process that is not there. Now it waits for a pid that is actually alive, and the helper's grandchild writes its OWN pid and execs, so the file existing means the process existed. sh -c rather than a plain subshell because in POSIX sh, $$ inside ( ) is still the outer shell's pid, which would have written the wrong number. All three vacuous cases now fail, and all three passed before: pid file naming a dead process no live grandchild within 5s grandchild that exits at once no live grandchild within 5s empty pid file no live grandchild within 5s The other problem is that when it does fail, it has to say why. Soaking the previous fix turned up a different failure twice in 160 loaded runs -- the helper returning 0 rather than being killed -- and all the assertion said was "assert 0 == -9". That was not enough to tell a helper that ended early from a kill that never happened, and 45 further runs on an instrumented copy did not reproduce it, so the cause is still unknown. The failure now reports the return code, the pid file, whether the grandchild is alive, and both output streams, so the next occurrence is diagnosable rather than another dead end. Flake rate across everything measured: original 12 failures / 37 runs after the first fix 2 failures / 160 runs Still catches what it exists for -- with os.killpg(pgid, SIGKILL) replaced by process.kill(), the grandchild is orphaned and the test fails.
Contributor
Author
|
Second pass on the flaky-test fix — soaking my own fix found two more problems with it. The readiness wait was vacuous. It waited for the pid file to appear and asserted only that it existed. That is satisfied by an empty file, a stale number, or a grandchild that has already exited — in each case the test then asserts about a process that is not there. All three cases passed before; all three fail now:
It now waits for a pid that is genuinely alive, and the grandchild writes its own pid then And when it fails it now says why. Soaking the first fix turned up a different failure twice in 160 loaded runs — I am not claiming zero — the last measurement is only 45 runs and the earlier failure was rare. What I can claim is that it is much rarer and no longer silent. Still catches what it exists for: replacing CI green. |
MiguelReis23
changed the base branch from
refuse-unsafe-upload-names
to
master
September 15, 2026 15:04
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
2 participants
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.
Closes #40, #41.
Merge order — please read, this one is not conflict-free
Based on #45 (
refuse-unsafe-upload-names), because it extends the sameworkspace.py.It conflicts with #38 and #43, and I could not avoid it. I tried: the handler rewrite deliberately leaves the parse line alone, but that is not enough, because #38's edits land inside the regions this PR rewrites. Measured, with a control that fires:
Both are small and neither is a disagreement — the changes are orthogonal in intent and merely adjacent in the file:
convert.py— Make the Snakefile a function of a validated intermediate document #38 replaces the dataclasses with Pydantic models in lines 28–86; this PR replacestools/fetch_toolstarting at line 82. A five-line overlap where one block ends and the other begins. Keep both.main.py— Make the Snakefile a function of a validated intermediate document #38's change is a single line, and it sits inside the handler this PR rewrites. Take this PR's handler and re-apply that line with the workspace path, exactly as below.The one line that must not be re-applied verbatim
On the converter track (after #47) that line reads:
"."is correct there, because the handler has justchdir'd into the run directory. This PR deletes that chdir, so re-applying the line as written makes"."the uvicorn process's working directory — oneintermediate.jsonshared by every request in flight, which is precisely the hazard #47 exists to prevent. Verified: a combined tree resolved that way writes the document to the process cwd, and the whole combined suite still passes, so nothing catches it.The correct resolution is:
Verified in a scratch merge: with
BIOCHEF_WRITE_INTERMEDIATE=truethe document lands inside the workspace and the process cwd stays empty.And one scoping note
ws.cleanup()in thefinallyremoves the workspace, and thereforeintermediate.jsonwith it, at the end of every run unlessBIOCHEF_KEEP_WORKSPACE=true. That is deliberate — the document's job is to be the validated thing the Snakefile is generated from, and that job is done when the run is. Persisting it beyond the request is provenance (#18) and needs a run to outlive its response (#5). It is called out because a reader who has just merged #38 may reasonably expect the artifact to still be there afterwards, and it will not be.This branch does not run on 3.11
Measured, and worth knowing before CI ever reaches here:
Not a regression from this PR. This track branches from
master, which carries the PEP 701 nested-quote f-string atconvert.py:230that only #36 removes — so the two tracks currently disagree about the floor: the converter track declares 3.11 inrun.shand supports it, this one inheritsmasterand needs 3.12.It resolves itself once the tracks converge, because #36's fix is present by then. Recorded because #48 adds a 3.11 job, and if that reached this branch first it would go red for a true reason that has nothing to do with what this PR changes.
What was found first
Recorded in the first commit, against the real handler:
os.chdiris process-global and this is an async server, so the current directory belongs to everything in flight at once.The interleaving case is deliberately not a test, because it needs a timing window. It was measured instead: under one worker with parts below starlette's 1 MiB spool threshold the handler has no
awaitbetween the twochdircalls, so requests serialise and cannot interleave. Above it the part spools and the read becomes a realawait; with several workers it happens at any size, and one run's snakemake reads the Snakefile another run wrote.What this changes
os.chdiris gone.snakemake -s <file> -d <dir>takes the Snakefile and the working directory explicitly — relative paths in the rules resolve under-d, and the shell blocks run with that as their cwd, so the emitter's./{bin}convention is untouched. Nothing needs the process to be anywhere. Thefinallythen removes a directory instead of restoring global state, which is why the recorded failure has nothing left to act on.Containment is structural, not a check. Every path is opened relative to a descriptor held on the workspace for the run's lifetime, with
O_NOFOLLOWandO_CLOEXEC;check_nameguarantees a single component, so there is no/for a path to escape through. The descriptor is opened once — re-resolving the directory per call would reintroduce the race it exists to remove.Measured on this branch:
#41 is closed on the read path. Outputs were read with a bare
open()and base64'd into the response, so a tool that replaced its own output with a symlink had the target returned with HTTP 200.open_readrefuses it withELOOP.Uploads are exclusive, and the tools go in first, so an upload named after a binary is an error rather than a silent replacement of the thing about to be executed.
O_EXCLfires beforeO_NOFOLLOW, so a symlinked slot is refused on write asFileExistsErrorand on read asELOOP; both are mapped to 400.The tool cache is split out because per-run directories force it.
fetch_toolmemoises and returns early on a hit, so it never re-copied the binary — with one shared directory that made the copy persist between runs, and with a fresh directory per run it would simply be missing on the second. Bundles are pulled once intoBIOCHEF_TOOL_CACHE, staged as.partand moved withos.replaceso an interrupted pull cannot be read as complete (which is also where a digest check belongs, #9).fetch_toolkeeps its two-argument signature, so the parse never learns about workspaces and the existing stubs keep working.os.systemis replaced byPopenwith a timeout that kills the process group. This is not a detail. Killing only snakemake leaves the tool running and then blocks forever on the pipes the orphan holds — which is exactly whatsubprocess.run(timeout=N)does internally:The pgid is captured before the first wait, because
getpgidraises once the child is reaped. The whole thing runs in a threadpool:communicateblocks, and a service that cannot have two runs in flight cannot have the problem this PR is about.Verified end to end
Everything else in the suite stubs the run, so there is one test that does not — a real snakemake, a real tool binary, through the real handler:
48 tests pass.
One regression this had, found by adversarial review and fixed here.
materialise_toolscopied a binary per node rather than per binary, so two nodes naming the same executable made the second failO_EXCLand the request 500. That is 80 of the 176 catalogue operations —samtools sortintosamtools indexreturned 500 on this branch and 200 on its base. The oldfetch_toolhid it behind its memo, which returned early and skipped the copy; splitting the cache out removed that side effect and nothing replaced it. No test used more than one tool node, which is why it got through; there is one now.Configuration, not feature flags
BIOCHEF_RUN_ROOT,BIOCHEF_RUN_TIMEOUT(finite, 900s),BIOCHEF_KEEP_WORKSPACE(off),BIOCHEF_TOOL_CACHE. All default to the safe value, and all change where or how long, never whether. A switch that turned containment off would be a documented way to reinstate #39 and #40.The threat model this assumes
Worth stating, because an earlier version of this PR reasoned from the wrong one and dismissed a real finding as a result.
The tool binary is untrusted. It is arbitrary code pulled from a registry and run against whatever the deployment can see. So "the attacker already executes code on the host" is the ordinary situation here, not an escalation — and where this service is meant to run, next to data that may not leave, the HTTP response body is the way out.
Under that model the containment is about what a running tool can reach, not only what a request can name. Two consequences, both now handled:
O_NOFOLLOWcloses the symlink form of A tool can exfiltrate any readable file by replacing its own output with a symlink #41. It does not close the hard-link form, because a hard link is another name for the same inode and is indistinguishable from the original. Verified before the fix: a tool that hard-links an outside file into its own output slot had the contents returned byopen_read. A file this workspace created has exactly one link, so the check is onefstat.snakemake -dis an origin and not a jail. That is unchanged and is E2. Container Runner #15, but its priority is higher under this model than the "remote HTTP client" framing suggests: confining a running tool is the central control rather than a later refinement.What a client can still do
snakemake -dis an origin, not a jail: a rule whose output is../../escapedwrites outside and snakemake exits 0. This PR removes the client's ability to declare such a path; confining a running binary needs E2. Container Runner #15.Content-Lengthor a proxy limit. C3. Execution hygiene #11 lists an upload size limit as a deliverable; that bullet should record why it cannot be done here.Refs #40, #41, #9, #11, #15, #39.