solx 1.0.3: keep renews entries you can write but don't own; count every failure - #48
Merged
Conversation
…ilure
`solx keep` renewed timestamps with `filetime::set_file_times`, which always
hands `utimensat` an explicit `[timespec; 2]` - even for "now". Per
utimensat(2) that form requires ownership of the entry, while the NULL-times
form plain `touch` uses requires only write permission. Every
collaborator-owned file in a shared /scratch project tree therefore failed
with "Operation not permitted (os error 1)" and was left to be purged, at
mode 0777 in a directory the caller owns. Call
`utimensat(AT_FDCWD, path, NULL, 0)` directly instead: same semantics ("both
stamps to now" is all keep ever wants), the permission rule touch gets.
The failure counter hid the blast radius: a batch assigned `errors = 1`
rather than accumulating, and each message overwrote the last, so a shard
where all 2000 files failed reported one failure and one path. Count
failures per entry, keep the first message plus "(and N more in this batch)",
and count `files_touched` as entries that actually got fresh stamps rather
than entries attempted - a failed pass could otherwise read as a successful
one.
Also renew directories, the flagged directory itself included, reported in a
new `dirs_touched` field. Enumeration was files-only and touching a file does
not move its parent's mtime, so the very directory Sol flagged could never be
refreshed. Symlinks are still neither touched nor followed.
Verified on Sol's BeeGFS /scratch against a 2,080-entry tree owned by another
user: 1.0.2 renewed none of it and reported a single failure; this renews
1,386 files and 694 directories with zero failures and nothing left stale.
Release: bump CLI + skill to 1.0.3, refresh Cargo.lock, move CHANGELOG
[Unreleased] -> [1.0.3], bump coverage.md. Skill guidance is unchanged, so
this is a CLI-only release.
Closes #47
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
Replace the inferred renewal figures with a before/after measurement taken on a fresh foreign-owned /scratch sample, run with the musl artifact CI built for this PR rather than a local glibc debug build - the shipped binary is what DEVELOPMENT.md's CLI-only release gate asks to exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Serial-mode progress output and documented dirs ordering rely on invariants that aren’t currently enforced and can mislead users or become fragile over time.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR ships solx v1.0.3 to fix solx keep renewal semantics on shared /scratch trees by switching to a utimensat(..., NULL, ...) “touch now” implementation (write-permission-based), counting failures per entry, and expanding renewal to include directories (so flagged directory mtimes can be refreshed).
Changes:
- Replace
filetime::set_file_times(now, now)with a directutimensat(AT_FDCWD, path, NULL, 0)touch to renew writable-but-not-owned entries. - Count renewal failures per entry (not per shard), preserve the first failing path, and add a
(and N more in this batch)suffix for batch-wide failures. - Enumerate and touch directories in addition to files; add
dirs_touchedto JSON output and update docs/tests accordingly; bump versions to 1.0.3.
File summaries
| File | Description |
|---|---|
| solx/src/keep.rs | Implements utimensat(NULL) touch, per-entry failure counting, directory enumeration/touching, and updated output counters. |
| solx/tests/cli.rs | Extends end-to-end keep test to validate directory renewal and dirs_touched. |
| solx/DEVELOPMENT.md | Updates design-decision docs for directory enumeration, utimensat(NULL) touch, and counting semantics. |
| solx/Cargo.toml | Bumps version to 1.0.3; moves filetime to dev-dependencies. |
| solx/Cargo.lock | Updates lockfile version metadata for 1.0.3. |
| skills/sol-skill/SKILL.md | Bumps shared skill version line to 1.0.3. |
| docs/solx.md | Documents directory renewal, writable-but-not-owned renewal, utimensat(NULL) semantics, and updated JSON counters. |
| docs/coverage.md | Bumps version and records verification notes/coverage rows for the new keep behavior. |
| CHANGELOG.md | Adds 1.0.3 release notes and updates footer links. |
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on #48, both comments actioned. The serial per-directory line printed `ok {enumerated} files` even when every touch in that directory had been refused - the same shape of under-reporting this PR exists to remove. Track a per-directory `Renewal`, print what actually got renewed, and drop the `ok` tag when anything failed. Exercised end to end against a foreign-owned non-writable directory on Sol: `fail 1 files 2 dirs · 1 failed` where the old line read `ok 1 files 3 dirs`. Also restate `enumerate_dir`'s contract as what is load-bearing - `dirs` holds the directory itself plus every subdirectory - rather than promising the walker's yield order. Nothing touches an entry twice either way, so the seed-the-root-and-skip-it alternative would only add a double-count path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
Same content, 36 doc lines down to 21. The long-form rationale for the utimensat form and the counting semantics lives in solx/DEVELOPMENT.md; the docstrings keep the one-line why and drop the restatement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
Topline rule pass over what this PR added: drop the development-note asides
("before the utimensat fix", "Before v1.0.3 solx keep used ...") so each
comment and doc paragraph reads as current behavior, and tighten the three
design bullets in solx/DEVELOPMENT.md to the density of their neighbors.
Formatting rules were already clean on the added lines - no unicode dashes or
arrows, no uncommaed e.g./i.e., no bare URLs, and the only lines over 100
characters are coverage.md table rows, which the file's existing rows match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX
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.
Closes #47.
Bug 1 - renewal required ownership,
touchrequires only write permissionkeeprenewed withfiletime::set_file_times, which always handsutimensatan explicit[timespec; 2]- even when the value is "now". Perutimensat(2) that form requires ownership; the NULL-
timesform plaincoreutils
touchuses requires only write permission. Everycollaborator-owned entry in a shared
/scratchproject tree therefore failedwith
Operation not permitted (os error 1)and was left to be purged, even atmode
0777inside a directory the caller owns.touch_nownow callsutimensat(AT_FDCWD, path, NULL, 0)directly. "Bothstamps to now" is the only thing
keepever wants, so this replacesset_file_timesunconditionally rather than acting as an EPERM fallback -strictly more permissive, semantically identical.
filetimemoves to[dev-dependencies](still used by the tests).Confirmed at the syscall level on Sol's BeeGFS
/scratch, on a foreign-owned0777file and directory:Bug 2 - the error counter hid up to
BATCHfailures per sharderrors = 1was an assignment, not an accumulation, and every messageoverwrote the last, so a shard where all 2000 files failed reported one
failure and one path. Failures are now counted per entry, and the message
keeps the first path plus
(and N more in this batch)so a shard-wide failureis legible without 2000 lines. The per-batch count is propagated in both the
pool worker and the serial path (
failures += errs, not+= 1).files_touchedalso counted entries attempted, including failures - theissue's
"files_touched": 34205next to"failures": 7for ~19,700unrenewed entries. It now counts entries that actually got fresh stamps; an
entry deleted between the walk and the touch is neither renewed nor a failure.
The "minor / separate question" - directory timestamps
Fixed rather than documented. Enumeration was files-only, and touching a file
does not move its parent's mtime, so a directory's stamp only ever moved when
an entry was added or removed - the very directory Sol flagged could not be
refreshed.
enumerate_dirnow returns aWalk { files, dirs, msg }(
find DIR -type fplusfind DIR -type d, the flagged directory first), andthe touched directories are reported separately:
{ "renewed": true, "dirs": 1, "files_touched": 1386, "dirs_touched": 694, "failures": 0 }dirskeeps its meaning (flagged directories the plan kept);dirs_touchedis new, so no existing field changes shape. Human summary:
done N flagged dirs · touched X files + Y dirs. Symlinks are still neithertouched nor followed.
Verification
Measured on a Sol compute node, BeeGFS
/scratch, with the musl binary CIbuilt for this PR (
solx-x86_64-linux-musl,--version1.0.3) - the shippedartifact, not a local debug build. Sample: 50 flagged directories under
.../papers/raw/arxivholding 150 entries owned by a labmate (100 files + 50directories, mode
0777), all 150 stale beforehand. Both binaries run over thesame sample,
-j 4:FAIL touch ... Operation not permittedlinesfiles_toucheddirs_touchedfailuresAt the syscall level the counter understatement is the same shape the issue
measured, two orders of magnitude smaller: 50 reported failures standing for
150 entries that were not renewed.
Crate gates:
cargo fmt --all --check,cargo clippy --all-targets -D warnings,cargo test(107 unit + 40 end-to-end, all passing). New vectorscover directory renewal, the root-plus-subdirs walk, symlinked directories,
per-entry failure counting with the
(and N more in this batch)suffix, andthe missing-path skip counting as neither renewed nor failed.
Release
Bundled as 1.0.3 per the #42 precedent:
Cargo.toml+SKILL.mdversion:,Cargo.lockrefreshed, CHANGELOG[1.0.3]with the footer link,docs/coverage.mdbumped with three new matrix rows. Skill guidance isunchanged apart from the shared
version:line, so this is a CLI-onlyrelease - the crate suite plus the L3 CLI smoke above are the gate, no skill
eval re-run. Not tagged: tagging stays with the maintainer after the L3
walk-through.
One deliberate omission:
skills/sol-skill/references/scratch.md's emergencyrecipe (
find ... -type f | xargs touch -a -m -c) calls itself "the sameprimitive
solx keepuses internally", which is now half-true since keep alsotouches directories. Editing it would make this a skill-touching release and
force an eval re-run for a one-line recipe, so it's left for the next release
that touches the skill.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AnxGBb2nDMiJrGfMynaDVX