The remote HardlinkTree refused every target that already existed, because sftp attrs carry no inode and it could not tell its own earlier link from a stranger's file. On the exec tier the backend can run stat on both sides, so a target that is already a link to its source is now skipped the way the local backend skips it. The sftp tier is unchanged.
Decisions, from the grill on the ticket:
Link first, compare on failure. The identity stats run only after Link has failed and an sftp lstat has shown the target exists, so the happy path keeps its one round trip and the diff stays inside the existing failure branch. One stat pair per collision; BatchStater (#2971) is the named upgrade if a season-pack retry is ever measured.
Same file meansfsops.SameFile between the target read unfollowed and the source read either way. The target is never followed, so a symlink planted there reports its own inode and never passes. The source is matched both ways because link(2) follows a symlinked source on macOS and not on Linux, so the link an earlier run made carries either identity depending on the host; one stat over both paths also reports each file's type, and only a symlinked source pays a second, followed stat. The local backend got the same two-way match, which it had lacked. Only a cancelled job or a lost connection passes through as the plan's error. Every other stat failure — the sftp tier, a source that vanished after the plan was built — reads as "not the source" and refuses with ErrTargetExists, because any other error would reach link mode's generic create-error path and, with the regular-mode fallback on, add the torrent in regular mode into the matched torrent's path, which is what #2942 prevents.
A skipped target is counted in SkippedExists and never recorded in Files, matching the local contract, so a later failure in the same plan cannot roll back a link another run owns (discussion #2282). That contract is now stated on HardlinkTree in the Backend interface for every implementation, and the local backend was brought in line with it: link(2) does not follow a symlinked source on Linux (it does on macOS), so targetSatisfiesPlan now matches the source read either way instead of refusing the link an earlier run made.
The reuse count rides the existing cross-seed created link tree log line as skippedExists, which covers the local backend too. The cross-seed result message is unchanged: linkedFiles is the plan size, so files: N/N stays accurate for an all-skipped plan.
This also fixes a symptom hit during the #3238 field test: a link tree left on disk without its torrent made cross-seed unable to recreate that torrent, because its own tree occupied the target it would build.
Closes #3237.
Breaking change
No breaking change.
Behaviour change
On a remote instance where qui has the exec tier, a cross-seed whose link target already holds qui's own link to the source now completes instead of failing with hardlink_error. A target holding any other file still fails, with no regular-mode fallback. The sftp tier behaves exactly as before.
How has this been tested?
Red-green at each seam, all five new tests run on the exec tier (newExecBackend skips off Linux, so they ran in a golang:1.27 container here and run in CI):
TestHardlinkTree_ExecTierSkipsOwnLink: a pre-existing link to the source is skipped, counted, not recorded, and both targets end up linking their source.
TestHardlinkTree_ExecTierStrangerStillRefuses: a different file at the target still answers ErrTargetExists, the link made before it is rolled back, the stranger's file is untouched.
TestHardlinkTree_ExecTierSymlinkAtTargetRefuses: a symlink to the source at the target is refused.
TestHardlinkTree_ExecTierSkippedLinkSurvivesLaterFailure: a skipped link is still in place after a later file in the same plan fails.
TestHardlinkTree_ExecTierReusesWholeTree: the same plan applied twice reports Created=0 SkippedExists=2, and removing that result is a no-op.
TestHardlinkTree_ExecTierSymlinkedSourceReuses: a plan whose source is a symlink is recognised on a re-run, because both sides are read unfollowed the way link(2) made the link.
TestTargetIsSource_JobStoppedDuringStatIsAnError: the connection is opened and probed, the fake then hangs every command, and a job stopped while the stat runs surfaces as the context's error, not as ErrTargetExists.
TestHardlinkTree_ExecTierMissingSourceStillRefuses: a source removed after the plan was built, with an old link at the target, still answers ErrTargetExists rather than an error link mode would turn into a regular add.
TestHardlinkTree_StaleStoredTierStillReusesOnExecHost: a row that still says sftp against a host that answers exec reuses the link on the first try, because the live connection decides the tier.
TestHardlinkTree_LstatFailureAfterLinkIsReported: when the lstat after a failed Link fails for a reason other than an absent target, that reason is the error, not the Link failure. Skips as root, so it runs on macOS and not in the container.
TestTargetSatisfiesPlan (local backend) gains two rows: a symlink planted at the target still refuses, and the link an earlier run made from a symlinked source reads as done. Both pass on macOS and Linux, which answer link(2) on a symlink differently.
TestHardlinkTree_ExistingTargetFailsAndRollsBack keeps its assertions and its comment, but its fake now refuses exec: the pool reads the tier from a probe of the live connection, and every test fake is ExecGNU, so before this change the test only passed because HardlinkTree never asked for identity. It now exercises the sftp tier it names, on Linux and macOS both.
Field test, exec tier (passed)
Run on the smoke instance with the pr-3322 image (/api/version reports it) against the same seedbox as the #3238 field test, on the exec tier (identity=true). Paths, release names and infohashes are masked per the repo rule.
Fresh build first: a cross-seed search found a match on a second tracker for a single-copy release, and the apply answered:
Then the reuse case this PR adds: the new torrent was removed from the client with its files kept, so the link tree stayed on disk with no torrent, and the same cross-seed was applied again once the removal was visible:
remove (keep files) 16:49:57Z
re-apply 16:50:57Z -> status: added_hardlink (2s)
same save_path, progress=1, downloaded=0, seeding again at 16:50:59Z
On pr-3238 the same setup answered hardlink_error: target already exists (recorded in the #3238 field test). The torrent was off its tracker for 62 seconds.
Documentation
No change in this PR. The hardlink-mode page's sentence that a remote instance cannot compare file identity becomes capability wording once the SSH setup screen exists; until then AGENTS.md sends a docs change that describes only remote instances to #3330, where it is listed as an item for this PR.
Performance
No change on the happy path: Link succeeds and nothing new runs. On a collision the remote backend now pays one exec session (a single stat over both paths) on the exec tier before deciding, where it previously refused after the sftp lstat; the sftp tier pays one pool.Exec call per collision that answers ErrNoExec without opening a session, because the live connection decides the tier (the stored row lags a key that just gained a shell, and would refuse the first reuse after it). A collision is rare and ends the plan either way except in the reuse case this PR adds, where the cost buys a successful cross-seed instead of a failure.
The one shape where the per-file cost compounds is a retry that collides on every file: a tree already on disk for a 100-file season pack costs, per file, a failed Link, an sftp lstat and one exec session, so on a host 100 ms away roughly 300 round trips and half a minute for an add that creates nothing. Before this PR the same retry failed on its first file. That shape is named in a ponytail: comment with BatchStater (#2971) as the upgrade, which could answer every identity in two finds before the first Link, and is left unmeasured until someone has that shape to measure. No benchmark taken.
Checklist
My PR title follows the Conventional Commits format (it becomes the squashed commit message)
If this changes the database schema, I have added migrations for both SQLite and PostgreSQL
Yes. Claude (Fable 5.1, via Claude Code) wrote the implementation and tests from the decision ledger on #3237, which came out of a grill with me; I reviewed every line and the container and local test runs.
Reviewing files that changed from the base of the PR and between 9adcf30d9b79d09f90da66860cd315a9ef7762cb and 68933e2620b18c6c44360106ed84adc28e5cbd6f.
📒 Files selected for processing: 3
internal/fsops/backend.go
internal/fsops/remote/remote.go
internal/fsops/remote/remote_write_test.go
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
📝 Walkthrough
The change defines when hardlink-tree operations can reuse existing targets. Local identity checks compare the target with both the followed and unfollowed source. The remote exec tier reuses verified links, while SFTP-only operations continue to refuse existing targets. Tests cover collisions, rollback, symlink cases, cancellation, and connection-tier selection. The link-tree log now reports the number of skipped targets.
Priority: ➖ Normal
Merge Risk:⚪ Minimal · up to 68933
The reported lstat error-handling issue is fixed. No actionable merge-blocking risk remains after normal checks.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
@coderabbitai The outside-diff finding on remote.go:672 is taken: fixed in ff15fa79. After a failed Link, only fs.ErrNotExist from the lstat that follows means the Link error is the whole story; a lost connection, a cancel or a permission failure there is now reported as itself, the same rule the identity check after it already follows. TestHardlinkTree_LstatFailureAfterLinkIsReported pins it; it skips as root, since root ignores directory modes, so it runs on macOS and not in the Linux container.
chatgpt-codex-connector[bot]reviewed11d ago
chatgpt-codex-connector[bot]11d ago
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ff15fa7948
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
@nitrobass24 Verified the code change. Only fs.ErrNotExist preserves the Link error; other lstat errors are returned through rollback.
TestHardlinkTree_LstatFailureAfterLinkIsReported covers the permission failure and skips when run as root. I inspected the test but did not run it.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
You are interacting with an AI system.
s0up4200reviewed10d ago
s0up420010d ago
This review was generated by AI.
Standards
1. The docs change belongs in #3330 for now
The change to documentation/docs/features/cross-seed/hardlink-mode.md:22 describes only remote instances. AGENTS.md:80 on develop says:
Until the SSH setup screen (#2917) ships, a documentation change that describes only remote instances goes in #3330, not in the PR. Add it there as an item with the PR number.
That rule landed in #3331 this morning, after you opened this PR, so you did not miss it. Netlify publishes getqui.com from main. If the change stays in this PR, it goes live with the next release, before the setup screen exists. Please revert the docs hunk and add the new wording to #3330 as an item for #3322.
2. A "must not" sentence in the contract comment
internal/fsops/backend.go:86 says "a rollback must not remove it". docs/standards/simplicity.md:27 says:
An invariant a future change must hold is a test, not a sentence with "must not" in it.
TestHardlinkTree_ExecTierSkippedLinkSurvivesLaterFailure (internal/fsops/remote/remote_write_test.go:493) already holds this rule for the remote backend. You can delete the sentence, or make it name that test.
Spec
3. A missing source falls back to a regular add instead of refusing
The interface comment says that every implementation must "refuse the whole plan with hardlinktree.ErrTargetExists otherwise". The exec tier does not refuse when the source is gone:
An old link tree occupies the target.
Someone deletes or moves the source file after qui builds the plan.
Link fails, and the lstat finds the target.
targetIsSource runs stat over both paths (internal/fsops/remote/remote.go:727). GNU stat exits non-zero for the missing source, and identities returns pathError("stat", ...) (remote.go:753).
HardlinkTree fails with that error, not with ErrTargetExists.
In link mode, only ErrTargetExists goes to the refusal (internal/services/crossseed/link_mode.go:451). Every other error goes to handleCreateError. With the regular-mode fallback on, that is handleFullRecheckFallback (link_mode.go:274), and qui adds the torrent in regular mode into the path of the matched torrent. #2942 exists to prevent this result. On develop, the remote backend refused in this case, and the local backend (targetSatisfiesPlan) still refuses.
The race is narrow, but the two backends give different answers. I suggest this fix. In targetIsSource, return an error only for a cancelled context or fsops.ErrConnectionLost. Read every other stat failure as "not the source", so the caller refuses with ErrTargetExists. A test that removes the source after the target exists would pin it.
4. The cancelled-context test does not reach the stat
TestTargetIsSource_CancelledContextIsAnError cancels the context before it calls targetIsSource, so it stops at the ctx.Err() check at the top of b.exec. It does not show what happens when the job stops during the stat. Amendment 1 on #3237 describes that case.
Standards: 2 findings, the worst is the docs change that has to move to #3330. Spec: 2 findings, the worst is a missing source that falls back to a regular add instead of refusing.
All four taken, in a081097d with develop merged on top (68933e26).
Docs — reverted to develop; the capability wording is now an item on #3330 for this PR. Thanks for noting the rule landed after I opened this.
The "must not" sentence — the contract comment names TestHardlinkTree_ExecTierSkippedLinkSurvivesLaterFailure instead.
Missing source — right, and the worse of the four: any error other than ErrTargetExists reached handleCreateError, and with the fallback on that is a regular add into the matched torrent's path. targetIsSource now passes through only a cancelled job or fsops.ErrConnectionLost; every other stat failure reads as "not the source" and refuses, which is what develop did on both tiers. TestHardlinkTree_ExecTierMissingSourceStillRefuses removes the source after an old link sits at the target and expects ErrTargetExists.
The cancellation test — replaced by TestTargetIsSource_JobStoppedDuringStatIsAnError: it opens and probes the connection with a Stat, switches the fake to ExecHang, and stops the job while the stat is running, expecting the context's error rather than a refusal.
Both new tests are exec-tier and so Linux-only; my local Docker daemon is wedged today, so they ran in CI rather than in a container here. The ledger on #3237 carries the three amendments.
nitrobass2410d ago
Field-tested live on the pr-3322 image against the seedbox on the exec tier; the record is in the body under "Field test, exec tier (passed)". The reuse case — a link tree on disk with its torrent removed, then the same cross-seed applied again — answers added_hardlink onto the occupied tree in two seconds, where pr-3238 answered hardlink_error for the same setup.
Codex Review Summary
This comment shows the latest Codex review activity on this pull request.
68933e2ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.