Decide the rewritten PCAP's mode instead of inheriting the umask (#98)

[AcoPiper]

Summary

write_atomic staged the rewritten capture with std::fs::write and renamed it into place. The rename puts the temporary's inode at the destination, so the mode of a file that ships in the output bundle was 0o666 & ~umask — set by the ambient umask of whoever ran multifold, rather than being a property of the artifact. The byte-identical fast path renames too, so a rewrite that changed no bytes still changed the mode.

src/pcap.rs:

• A new stage_tmp creates the temporary through OpenOptions with an explicit .mode(0o600), writes the bytes through that handle, and calls set_permissions(0o644) on it before the rename. open(2) masks its mode argument with the umask and chmod(2) does not, so the explicit call is what makes the final mode a decision; the creation mode is a ceiling that keeps the incomplete file from being world-writable while the bytes stream in, which std::fs::write's 0o666 left to the umask. The std::io::Result<()> signature is unchanged and a failed permissions call is propagated, with the temporary removed on that path as on the others.
• The doc comment states why 0o644 (a decided value for a bundle artifact the invoking user reads, holding no secret — not a mode carried over from the input) and why the explicit call is needed on top of the open-time mode.
• A comment at the rename records that the bytes and the rename are deliberately not flushed, and why: a rewritten capture is a terminal artifact of a finishing run, nothing reads it back to resume from, and a crash there costs the whole bundle rather than this one file.

src/activity.rs:

• A comment at the ?? in the join_next loop records that the early return, and the abort of the remaining activities that follows from it, is intended — only a fatal error reaches it, since a non-zero command exit returns Ok and still drains the set — and that what those tasks left running is reclaimed by teardown on a best-effort basis rather than reliably, because teardown_inner discards its failures and has no reach over the local sshpass child. The control flow is unchanged.
• A schedule-time overflow does not reach that ??: logical_offset_to_real and the checked_add_signed after it are evaluated in the spawn loop, so an overflow returns from run before join_next is polled, dropping the JoinSet at a point the loop's comment never executes. It is the same deliberate abort — earlier iterations have already put tasks in the set by then — so it is recorded at the loop it actually leaves from rather than listed at the one it cannot reach.

Closes #98

Test plan

• rewrite_normalizes_mode_on_byte_identical_path — builds a single-record capture in a tempfile::tempdir(), sets it to 0o600, runs rewrite_timestamps, and asserts the bytes are unchanged and mode() & 0o777 == 0o644.
• rewrite_normalizes_mode_on_reassembly_path — same assertion on the path that actually sorts and reassembles, so the mode is normalized whether or not the capture's bytes change.
• write_atomic_removes_the_tmp_when_the_rename_fails — the temporary does not outlive a failure after it exists, and the destination is left as it was.
• Neither test calls umask(2), spawns a child to set one, or otherwise depends on the umask it runs under.
• cargo fmt -- --check --config group_imports=StdExternalCrate passes.
• cargo clippy --all-targets -- -D warnings passes.
• cargo test passes (391 passed, 0 failed).
• CI green on all jobs — Markdown, Quality Check, Test, the two AC-0 pipeline jobs, and Docker E2E.

Notes

• No new dependency; std::fs::OpenOptions, std::os::unix::fs::OpenOptionsExt, and std::os::unix::fs::PermissionsExt are enough, and tempfile stays a dev-dependency.
• Unix-only, with no cfg-gated fallback, as specified.
• Hardening the temporary's fixed, predictable name against a hostile output directory (create_new(true) / O_EXCL) is out of scope per the issue and worth filing separately; the 0o600 creation mode neither depends on it nor substitutes for it.
• No CHANGELOG.md entry: the file has never carried a release, so there is no user of a last release who could observe this.
• One limitation, stated so nobody reads more into the tests than they carry: pinning the resulting mode cannot on its own prove umask-independence, because under the common 0o022 umask the old std::fs::write path also produced 0o644. The tests discriminate on a machine with a 0o077 umask and are a value pin and regression guard elsewhere; umask-independence is carried by the shape of the code.

[AcoPiper]

[Reviewer Round 1]

src/activity.rs:241-244: a schedule-time overflow cannot reach this ??. target_time.checked_add_signed(...).context(...) is evaluated in the spawn loop, before join_next; if it happens after earlier activities have been spawned, run returns from that loop and drops the JoinSet without executing this comment. Please remove it from this list (or document that separate early-return path where it occurs) so the rationale accurately describes the behavior it is meant to record.

[AcoPiper]

[Review Verdict Round 1: NOT_APPROVED]

[AcoPiper]

[Author Round 1]

src/activity.rs:241-244 — a schedule-time overflow cannot reach this ?? — Fixed

You are right, and I checked the control flow rather than taking it on faith. logical_offset_to_real (src/activity.rs:178) and the checked_add_signed(...).context(...) after it (src/activity.rs:179-181) are both evaluated inside the spawn loop, so an overflow returns from run before join_next is polled for the first time. The comment listing it described a failure the code it annotates cannot see.

The claim came from the issue's own prose, which lists the overflow alongside the Docker and SSH failures. That paragraph is a paraphrase of the behaviour and the code is what governs, so I have corrected it here rather than reproducing it.

I took the second of the two options you offered — documenting the path where it occurs — because the overflow really does abort activities: by the time it can fire, earlier iterations of the spawn loop have already spawned tasks into the set, and returning drops it exactly as the join_next loop does. Dropping the mention entirely would have left that path undocumented while the issue's whole subject is that an omission and a decision read identically in the source.

So:

• The ?? comment now lists only what arrives there — a Docker exec failure, an SSH spawn failure, or a panic.
• A new comment above the spawn loop records that a schedule-time overflow returns from there, dropping tasks and aborting whatever earlier iterations already spawned, and that this is the same deliberate abort arriving before the join_next loop rather than from within it.

Control flow is unchanged, per the issue's constraint — both changes are comments.

The PR description has been updated to match.

• cargo fmt -- --check --config group_imports=StdExternalCrate — passes
• cargo clippy --all-targets -- -D warnings — passes
• cargo test — 391 passed, 0 failed

[AcoPiper]

[Reviewer Round 2]

Approved.

The Round 1 item is resolved: the schedule-time-overflow rationale now documents the spawn loop where that error actually returns and drops the JoinSet; the join_next comment now lists only failures that can reach its ??.

I found no remaining or new findings. stage_tmp creates the temporary through a 0o600 OpenOptions handle, writes through that handle, then explicitly applies 0o644 before rename. The cleanup covers failures from writing or setting permissions as well as rename failures, and the tests exercise both the byte-identical and reassembly paths while pinning the final mode. The PR also retains Closes #98 and a complete test-plan checklist.

[AcoPiper]

[Review Verdict Round 2: APPROVED]

[AcoPiper]

Suggested squash commit

Title

Decide the rewritten PCAP's mode explicitly

Body

`write_atomic` staged the rewritten capture with `std::fs::write` and
renamed it into place. The rename carries the temporary's inode, so
the mode of a file that ships in the output bundle was whatever
`0o666 & ~umask` happened to be for whoever ran `multifold` — not a
decision anyone made about the artifact, and not the same on two
machines. The byte-identical fast path renames too, so a rewrite that
changes no bytes still changed the mode.

Stage through an `OpenOptions` handle opened at `0o600` so the
incomplete file is never wider than owner-read-write while the bytes
stream in, then `set_permissions` to `0o644` before the rename.
`open(2)` masks its mode argument with the umask and `chmod(2)` does
not, so the explicit call is what makes the final mode a decision
rather than an inheritance; the creation mode is a ceiling, not a
value.

The comments alongside record choices that were previously
indistinguishable from omissions in the source: that the staged bytes
and the rename are deliberately not flushed, and that the early return
in `activity::run`'s `join_next` loop aborts the remaining activities
on purpose, with only best-effort cleanup behind it. A schedule-time
overflow leaves through the spawn loop instead, before `join_next` is
ever polled, so that path is recorded where it actually returns from.
No control flow changes.

Closes #98