MILAB-5933: sink to a temporary file so an in-place overwrite stops crashing - #1823
MILAB-5933: sink to a temporary file so an in-place overwrite stops crashing#1823vgpopov wants to merge 5 commits into
Conversation
… crashing
A workflow may read a file and write the result back over the same path, which
is what read_csv followed by write_csv on one path does. The read is lazy, so
sinking straight to the target truncates a file polars is still reading. Local
filesystems hide that behind cached pages; the k8s deploy's CephFS workspace
does not, and ptabler died there with a bus error and exit 135.
Sink to a sibling '.ptabler-partial' file and move it into place from a chained
task, after every sink has been collected.
os.replace swaps in a new inode, so the move carries the target's mode across
first: a workdir file the backend staged writable at 0o600 came back 0o644
without that, which would have broken the exec.builder { writable: true }
contract while looking like a fix.
write_frame sinks to a generated path inside its own frame directory and can
never collide with an input, so it keeps sinking directly.
🦋 Changeset detectedLatest commit: 1ef65b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # to file_path truncates a file polars is still reading. On a local filesystem | ||
| # that survives on cached pages; on a network filesystem the mapping goes away | ||
| # underneath the reader and the process takes SIGBUS. | ||
| temp_path = f"{file_path}.ptabler-partial" |
There was a problem hiding this comment.
Every writer for a target uses the same <target>.ptabler-partial path. If a workflow has two write steps for the same target, both sinks write to that shared path before either replacement task runs. Concurrent writes can corrupt the output, and even serialized writes leave the second replacement without its temporary file. The same path can also collide with a valid user file. Generate a unique temporary path for each sink.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ptabler/software/src/ptabler/steps/io.py
Line: 202
Comment:
**Temporary paths can collide**
Every writer for a target uses the same `<target>.ptabler-partial` path. If a workflow has two write steps for the same target, both sinks write to that shared path before either replacement task runs. Concurrent writes can corrupt the output, and even serialized writes leave the second replacement without its temporary file. The same path can also collide with a valid user file. Generate a unique temporary path for each sink.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| ctx.add_sink(sink_plan) | ||
| ctx.chain_task(lambda: _replace_preserving_mode(temp_path, file_path)) |
There was a problem hiding this comment.
The target's mode is applied only by the chained task after every sink succeeds. If a sink fails, that task is skipped, leaving a partial file behind with the sink's default permissions. If the workspace is accessible to another principal, output intended for a restrictive target such as 0600 can therefore be readable before the later chmod. Apply the intended permissions when creating the temporary output and clean it up on failure.
How this was verified: The sink creates the sibling output before the target mode is applied, while sink exceptions bypass all chained chmod, replacement, and cleanup work.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ptabler/software/src/ptabler/steps/io.py
Line: 206-207
Comment:
**Temporary output remains exposed**
The target's mode is applied only by the chained task after every sink succeeds. If a sink fails, that task is skipped, leaving a partial file behind with the sink's default permissions. If the workspace is accessible to another principal, output intended for a restrictive target such as `0600` can therefore be readable before the later `chmod`. Apply the intended permissions when creating the temporary output and clean it up on failure.
**How this was verified:** The sink creates the sibling output before the target mode is applied, while sink exceptions bypass all chained chmod, replacement, and cleanup work.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| self.assertTrue(os.path.exists(target)) | ||
| leftovers = [n for n in os.listdir(root) if n.endswith(".ptabler-partial")] | ||
| self.assertEqual([], leftovers, "the temporary sink file must be moved into place") |
There was a problem hiding this comment.
These tests only check that the target exists, retains its mode, and leaves no partial file. An empty, truncated, or malformed temporary output would still satisfy those assertions after replacement. Re-read the overwritten table and assert its rows and columns so the regression test verifies that the data remains correct.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ptabler/software/src/test/overwrite_test.py
Line: 31-33
Comment:
**Overwrite contents are untested**
These tests only check that the target exists, retains its mode, and leaves no partial file. An empty, truncated, or malformed temporary output would still satisfy those assertions after replacement. Re-read the overwritten table and assert its rows and columns so the regression test verifies that the data remains correct.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…n a run fails Review of the previous commit raised three things, all of them real. The partial name came from the target alone, so two steps writing one target shared a file: both sinks wrote it before either was moved into place, and the second move found nothing there. Each sink now gets a unique name from mkstemp, which also puts it out of reach of a name the workflow's own data uses. The target's mode was applied at the move, so a partial file for a target restricted to 0o600 sat under the umask for as long as the write took. mkstemp opens at 0o600 and the target's mode is applied before the first row is written, so that window is gone. The move re-applies it because polars may recreate the path rather than truncate the file. A sink that raised left its partial behind, and in a block's working directory that file is not inert — it is collected as part of the block's output. The workflow removes whatever is still registered when execution ends. The tests asserted the target existed and kept its mode but never read it back, so an empty or truncated result would have passed. They now check the rows and columns, and cover the two cases above.
|
All three findings were valid and are fixed in P1 — temporary paths can collide. Correct: the name came from the target alone, so two write steps for one target shared a partial file. Both sinks are collected before either is moved, so the second move found nothing there. Each sink now takes a unique name from P2 — temporary output remains exposed. Also correct, and the window was the whole write, not just an instant: the mode was applied at the move, so a partial file for a On cleanup — this matters more here than the comment suggests. A leftover file in a block's working directory is not inert: it gets collected as part of the block's output. Partial files are registered on the context and removed in the workflow's P3 — overwrite contents are untested. Fair. The assertions checked existence and mode but never read the table back, so an empty or truncated result would have passed. They now assert the rows and columns. Each fix is covered by a test I verified fails without it: Worth noting the second one: my first version of that test passed without the cleanup code, because an unknown table raises during
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1823 +/- ##
==========================================
- Coverage 55.33% 55.28% -0.06%
==========================================
Files 428 428
Lines 22883 22883
Branches 5240 5240
==========================================
- Hits 12663 12651 -12
- Misses 8647 8653 +6
- Partials 1573 1579 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The CI run on the first version of this change failed the two
exec.builder { writable: false } cases: they hand pt a file staged 0o400 and
expect the write to be refused, and pt wrote it anyway. A partial file walks
straight past that mode, because os.replace asks the directory for permission
and never the file.
Applying the target's mode to the partial file up front already closes the
hole — the sink itself is refused — but it is refused on a path the caller has
never heard of:
Permission denied: .data.tsv.91b2q4ga.ptabler-partial
So a target that refuses writes now skips the partial file and sinks onto
itself, and the error names the file the workflow asked for:
Permission denied: data.tsv
A path that does not exist yet refuses nothing; that write still goes through
a partial file, which is what the crash this change exists for needs.
| # A sink that raised leaves its partial file behind, and in a block's working | ||
| # directory that file is not inert — it is collected as part of the block's | ||
| # output. Whatever reached its target has already been moved off this list. | ||
| if ctx is not None and not lazy: |
There was a problem hiding this comment.
Lazy execution creates and registers each writer's partial file before returning the deferred StepContext, but this guard disables the only cleanup in that mode. If the caller does not run the chained replacement task, or sink collection fails, the partial file remains in the working directory and may be collected as block output. A cleanup mechanism is needed for deferred execution so ownership of these files is clear.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ptabler/software/src/ptabler/workflow/workflow.py
Line: 126
Comment:
**Lazy partials remain behind**
Lazy execution creates and registers each writer's partial file before returning the deferred `StepContext`, but this guard disables the only cleanup in that mode. If the caller does not run the chained replacement task, or sink collection fails, the partial file remains in the working directory and may be collected as block output. A cleanup mechanism is needed for deferred execution so ownership of these files is clear.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.… the context A writing step opens the file its sink will fill while the step runs, so a lazy call returns with those already on disk. The workflow cannot remove them there — the caller has not run the sinks yet — and nothing else did, so they stayed in the working directory, where a leftover file is collected as part of the block's output. Ownership is now stated rather than implied: cleanup_partial_outputs on the context removes whatever has not reached a target, the workflow calls it for a non-lazy run, and the lazy branch of execute says the caller owns them and names the method. Also corrects the note on re-applying the mode at the move. Polars truncates the file it is given rather than recreating it — same inode, mode intact — so that step re-applies what is usually the same mode and is there for the case where that stops holding, not because polars is known to drop it.
2 participants
Why
A workflow may read a file and write the result back over the same path —
read_csvthenwrite_csvon one path. The read is lazy, so sinking straight to the target truncates a file polars is still reading.A local filesystem hides that behind cached pages. A network filesystem does not. On the k8s+S3 e2e deploy, whose workspace is a CephFS RWX PVC, ptabler died:
135=128 + 7= SIGBUS — the mapping went away underneath the reader.What
Sink to a sibling
.ptabler-partialfile and move it into place from achain_task, which already runs aftercollect_all. No engine change needed._replace_preserving_modecarries the target's mode across first.os.replaceswaps in a new inode, so without that step the file comes back with the temporary file's mode — see below.write_framesinks to a generated path inside its own frame directory and can never collide with an input, so it keeps sinking directly.The mode is the subtle part
A naive temp+rename silently downgrades a file the backend staged writable, which would break the
exec.builder { writable: true }contract while looking like a fix. Measured with the chmod removed:test_overwrite_keeps_the_mode_the_target_hadfails loudly if anyone takes that line out again.Tests
Three new cases in
src/test/overwrite_test.py: the partial file is moved into place, the mode survives, and writing to a fresh path still works.Caveat
The crash reproduces only on a network filesystem, so this cannot be proven fixed locally — local runs pass with or without the change. What the tests demonstrate is that the truncate-under-read mechanism is gone and the mode regression is guarded.
Found while fixing the k8s e2e suites (#1809, milaboratory/pl#2194). It was only diagnosable after the k8s runner stopped reporting
-1for every failed job and surfaced the real135.Greptile Summary
This PR prevents lazy table readers from being invalidated by in-place writes: writers now sink into unique sibling files, preserve target permissions, replace the target after collection, and clean registered partial outputs after ordinary execution. The latest changes address all three previous findings by generating unique names, applying permissions before writing and cleaning failures, and asserting overwritten table contents.
Important touched terms:
sink_csv,sink_ndjson, orsink_parquet. Writers now direct these plans to partial outputs rather than their final targets..ptabler-partial. It receives sink data before an atomic replacement and is tracked for cleanup.StepContextwithout collecting sinks or running chained tasks. Partial files are now created during this mode, but their cleanup lifecycle remains incomplete.Confidence Score: 5/5
The PR appears safe to merge, with a non-blocking cleanup gap limited to deferred lazy execution.
The prior collision, permission-exposure, and missing-content-assertion findings are fully addressed. The remaining new concern is that lazy execution creates partial files but provides no automatic cleanup if deferred work is abandoned or fails.
Files Needing Attention: lib/ptabler/software/src/ptabler/workflow/workflow.py
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Read target as LazyFrame] --> B[Create unique sibling partial] B --> C[Apply target mode] C --> D[Build deferred sink plan] D --> E[Collect all sinks] E -->|success| F[Run chained replacement] F --> G[Atomically replace target] E -->|failure| H[Remove registered partials] D -->|lazy execution| I[Return StepContext] I --> J[Caller owns collection and chained tasks] J --> K[Cleanup lifecycle currently incomplete]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "[MILAB-5933]: Leave a read-only target t..." | Re-trigger Greptile