`rimport` destroys the inputdata link when the path given goes through a symlink
- Vorherrschende Sprache
- Python
- Sterne
- 0
- Forks
- 2
- Ø Merge
- 3 T. 2 Std.
- Gemergte PRs (30 T.)
- 2
Beschreibung
(Identified and written by Claude; I haven't yet verified.)
## Summary
If you name a file through a symlink that points into the inputdata tree, `rimport`
stages the file correctly but then replaces the original with a symlink **pointing at
itself**. The published file becomes unreadable (`ELOOP`), and `rimport` exits 1.
The staged copy is fine, so no data is lost — but the path everyone reads from is broken
until someone repairs it by hand.
## Impact
- The inputdata path is left as a self-referential symlink: `cat` reports
`Too many levels of symbolic links`, and `Path.exists()` returns `False`.
- Anything resolving that path — a CESM run, a download check, a later `rimport` — sees a
broken file.
- It is silent about the real cause. The run reports `Error relinking during rimport`,
which reads like a transient failure rather than a corrupted link.
Severity is bounded by two things: the staged copy under the staging root is intact and
correct, and the failure is detected (exit 1) rather than passing as success. It is a
recoverable break, not data loss.
## Reproduction
```bash
W=$(mktemp -d)
mkdir -p "$W/inputdata/lnd" "$W/staging"
echo data > "$W/inputdata/lnd/a.nc"
ln -s "$W/inputdata" "$W/plink" # a symlink pointing at the inputdata root
RIMPORT_STAGING="$W/staging" RIMPORT_SKIP_USER_CHECK=1 \
rimport -inputdata "$W/inputdata" "$W/plink/lnd/a.nc"
```
Observed:
```
'/plink/lnd/a.nc':
[rimport] staged /plink/lnd/a.nc -> /staging/lnd/a.nc
Deleted original file: /plink/lnd/a.nc
Created symbolic link: /plink/lnd/a.nc -> /staging/../plink/lnd/a.nc
rimport: error processing /plink/lnd/a.nc: Error relinking during rimport
EXIT=1
```
`/staging/../plink/lnd/a.nc` normalizes to `/plink/lnd/a.nc`, which is the symlink
that was just created. Afterwards:
```
$ ls -l "$W/inputdata/lnd/a.nc"
... a.nc -> /staging/../plink/lnd/a.nc
$ cat "$W/inputdata/lnd/a.nc"
cat: ...: Too many levels of symbolic links
$ cat "$W/staging/lnd/a.nc"
data # the staged copy is correct
```
Expected: either the same result as naming the file directly (staged, and the original
replaced with a working symlink into staging), or a clean pre-flight rejection. Not a
half-completed publish that breaks the path.
## Root cause
Two places compute "where does this file live relative to the inputdata root", and they
disagree when the path reaches the tree through a symlink.
| Site | Computation | Result for the repro |
|------|-------------|----------------------|
| `rimport:526` (`stage_data`) | `src.resolve().relative_to(inputdata_root.resolve())` — **resolved** | `lnd/a.nc` → stages to `/staging/lnd/a.nc` ✅ |
| `relink.py:244` (`replace_one_file_with_symlink`) | `os.path.relpath(file_path, inputdata_root)` — **lexical** | `../plink/lnd/a.nc` → links to `/staging/../plink/lnd/a.nc` ❌ |
`os.path.relpath` does no filesystem lookup, so it cannot see that `plink` and `inputdata`
are the same directory. It escapes the root with `..`, and joining that onto the staging
root walks straight back out to the original path.
`relink`'s own existence check does not catch it: at that moment `link_target` still
resolves to the original file, which does exist, so it proceeds to delete the original and
link over it. `check_relink` (`rimport:400-402`) notices afterwards and raises — after the
damage is done.
## Suggested fix
Make the two agree. The narrower change is in `relink.py:244`, which also covers `relink`
used standalone:
```python
relative_path = os.path.relpath(
os.path.realpath(file_path), os.path.realpath(inputdata_root)
)
```
This was tried against the reproduction above: the run then exits 0 and produces
`a.nc -> /staging/lnd/a.nc`, which reads correctly. The existing suite (341 tests) stays
green with it applied, so nothing currently depends on the lexical behaviour.
Worth considering alongside it: have `replace_one_file_with_symlink` refuse a computed
`link_target` that is not under `target_dir`, before deleting anything. That turns any
future instance of this class of bug into a clean refusal rather than a broken link, and
it is a cheaper invariant than keeping two path computations in step by inspection.
A regression test should cover the symlinked-root shape specifically; nothing in the
current suite exercises a path that reaches the inputdata tree through a symlink.
## Scope
- Reproduced identically on `main` and on the `fix-rimport-on-dir` branch. **Pre-existing;
not introduced by directory enumeration.**
- On the branch, the directory form reaches it too (`rimport /lnd`), because
enumeration resolves the named directory and then hands each discovered file to the same
relink path. Same single failure, same cause.
- Verified on Python 3.13.2. The mechanism is `os.path.relpath` versus `Path.resolve()`,
neither of which is version-sensitive, so the CI matrix (3.9–3.12) will behave the same.
## Recovery, if it has already happened
The staged copy is intact. Repoint the link by hand:
```bash
ln -sf "$STAGING/" "$INPUTDATA/"
```
Verified against the reproduction: the file is readable again afterwards, with the staged
copy as its target. Nothing needs to be re-staged.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Rechercherichtung
Beginne mit relink.py:244 und vergleiche dessen Pfadberechnung mit stage_data bei rimport:526; untersuche außerdem check_relink bei rimport:400-402. Führe die bereitgestellte Reproduktion mit einem symbolischen Root-Link und die bestehende Testsuite mit 341 Tests aus und füge anschließend einen Regressionstest für diese Pfadstruktur hinzu. Als abgeschlossen gilt die Aufgabe, wenn der Befehl erfolgreich beendet wird und die veröffentlichte Datei über einen funktionierenden Staging-Symlink weiterhin lesbar ist.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- python
- Bereich
- cli
- Issue-Typ
- Bug
- Schwierigkeit
- 2/5
- Geschätzter Aufwand
- 1-3 Stunden
- Aktivitätsstatus
- Aktiv
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 85/100