feat: a joined node keeps its chats' attachments in the pool #505

2026-10-19T13:24:12Z

rvnminers-A-and-N commented

2026-10-19 13:24:12 +00:00

Closes #500. Stacked on #503 (slice 1). Once that merges, this rebases onto dev, and its diff is just the commit on top.

A node that has joined another's pool as a client now keeps its chat attachments there too, so a chat on a worker+client node can attach files the way it does on a coordinator.

How

The coordinator keeps one client pin per file for this node. Several owners on the node can want the same file, though: the user, each chat that has it, and the draft of an attachment not yet sent (kept for a day). The node now remembers those itself, and turns them into that one pin:

If any owner holds the file for good, the pin is permanent.
Otherwise the pin lasts until the latest expiry among the owners.
When the last owner lets go, the pin is released.

Each change is made on the coordinator first and noted locally after. A failure between the two keeps a file longer, never shorter. When the coordinator doesn't have the file, the error is storage.ErrNotFound, so the existing attachment-pin queue drops that change instead of retrying it forever. While the node is away, a change fails with UNAVAILABLE, and the queue retries it once the node is back.

packages/nodedb: migration 0032_pool_file_holds and PoolFileHolds / HoldPoolFile / ReleasePoolFile. Holds are keyed by coordinator ID, like pool_jobs, and expired ones are pruned.
apps/sisyphusd/pool_files.go: *poolFiles maps owner holds onto the coordinator pin. The daemon also uses it as the assistant's attachment pin store.
Removed slice 1's refusals of chat attachments on a joined node.

Tests

The E2E TestAJoinedClientNodeKeepsItsFilesInThePool now drives a real chat on the joined node:
1. A draft attachment is pinned with an expiry.
2. Ask with it runs a private wordcount in the pool, and the pin becomes permanent.
3. After DeleteChat, the pin falls back to the draft's expiry.
Unit TestAJoinedNodesFilesAreKeptAsLongAsTheLongestHold covers the hold-to-pin mapping and each failure path.
TestTheHoldsOnFilesKeptInAPoolAreKeptAcrossReopening covers the nodedb part.
make cover: every statement is covered. gofmt and vet are clean.

Docs

docs/desktop-backend.md: attachments on a joined node.

Closes #500. Stacked on #503 (slice 1). Once that merges, this rebases onto `dev`, and its diff is just the commit on top. A node that has joined another's pool as a client now keeps its **chat attachments** there too, so a chat on a worker+client node can attach files the way it does on a coordinator. ## How The coordinator keeps **one** client pin per file for this node. Several owners on the node can want the same file, though: the user, each chat that has it, and the draft of an attachment not yet sent (kept for a day). The node now remembers those itself, and turns them into that one pin: - If any owner holds the file for good, the pin is permanent. - Otherwise the pin lasts until the latest expiry among the owners. - When the last owner lets go, the pin is released. Each change is made on the coordinator first and noted locally after. A failure between the two keeps a file longer, never shorter. When the coordinator doesn't have the file, the error is `storage.ErrNotFound`, so the existing attachment-pin queue drops that change instead of retrying it forever. While the node is away, a change fails with `UNAVAILABLE`, and the queue retries it once the node is back. - `packages/nodedb`: migration `0032_pool_file_holds` and `PoolFileHolds` / `HoldPoolFile` / `ReleasePoolFile`. Holds are keyed by coordinator ID, like `pool_jobs`, and expired ones are pruned. - `apps/sisyphusd/pool_files.go`: `*poolFiles` maps owner holds onto the coordinator pin. The daemon also uses it as the assistant's attachment pin store. - Removed slice 1's refusals of chat attachments on a joined node. ## Tests - The E2E `TestAJoinedClientNodeKeepsItsFilesInThePool` now drives a real chat on the joined node: 1. A draft attachment is pinned with an expiry. 2. `Ask` with it runs a private `wordcount` in the pool, and the pin becomes permanent. 3. After `DeleteChat`, the pin falls back to the draft's expiry. - Unit `TestAJoinedNodesFilesAreKeptAsLongAsTheLongestHold` covers the hold-to-pin mapping and each failure path. - `TestTheHoldsOnFilesKeptInAPoolAreKeptAcrossReopening` covers the nodedb part. - `make cover`: every statement is covered. gofmt and vet are clean. ## Docs `docs/desktop-backend.md`: attachments on a joined node.

excho0 reviewed

2026-10-19 13:59:05 +00:00

apps/sisyphusd/pool_files.go

excho0 commented

2026-10-19 13:59:05 +00:00

This persists the owner hold only after the remote pin update, which is good for avoiding premature release. There is still a lost-retry case for RemoveFile: the local API deletes the file catalogue row before calling Unpin; if the coordinator RPC fails, this row remains in pool_file_holds, but the file is no longer listed and there is no reconnect/startup path here that retries the pending release. The coordinator keeps the permanent client pin and quota indefinitely. Could we preserve the file entry until the remote change succeeds, or add a durable event-driven retry for pending hold changes, with a test for failed Unpin followed by reconnect? /cc @rvnminers-A-and-N

excho0 requested changes

2026-10-19 13:59:07 +00:00

excho0 left a comment

One recoverability gap remains around RemoveFile when the coordinator rejects or cannot receive Unpin: the durable user hold is left behind after the local catalogue entry is removed, with no reconnect/startup retry. I left an inline comment with the failure sequence and test request. /cc @rvnminers-A-and-N

rvnminers-A-and-N reviewed

2026-10-19 14:27:40 +00:00

apps/sisyphusd/pool_files.go

rvnminers-A-and-N commented

2026-10-19 14:27:40 +00:00

Good catch, thank you. That was a real hole. I went with your first option, keeping the file entry until the remote change succeeds, in 13e1b41.

RemoveFile still takes the file off the list first, so a file is never listed after its release has gone through. If the store then fails to let it go, the entry is put back exactly as it was. The error carries the store's own code, so a joined node that is away gets UNAVAILABLE rather than INTERNAL.
The user's hold row in pool_file_holds is written or deleted only after the coordinator call succeeds. A failed Unpin therefore leaves both the hold and the listing in place. Nothing is kept without the node showing it, and the next RemoveFile after reconnecting releases it.
I didn't add an automatic retry for user removals. The user asked for this removal and can see that it failed, so a background retry would remove a file they may have decided to keep after all. Chat attachments are different, because nobody is watching that release. They already go through the durable file_pin_operations queue, which retries on reconnect.

The test is TestAFileThatCannotBeLetGoStaysListed in api/local_files_test.go. It covers four cases:

A store that answers UNAVAILABLE makes the removal fail, and the file stays listed next to an unrelated one.
After "reconnecting", the same removal succeeds and only that file goes.
When re-listing also fails, the error says so.
When the list itself cannot remove the entry, the store is never asked to let it go.

The docs in desktop-backend.md describe the behaviour. Could you take another look?

Good catch, thank you. That was a real hole. I went with your first option, keeping the file entry until the remote change succeeds, in 13e1b41. - `RemoveFile` still takes the file off the list first, so a file is never listed after its release has gone through. If the store then fails to let it go, the entry is put back exactly as it was. The error carries the store's own code, so a joined node that is away gets `UNAVAILABLE` rather than `INTERNAL`. - The user's hold row in `pool_file_holds` is written or deleted only after the coordinator call succeeds. A failed `Unpin` therefore leaves both the hold and the listing in place. Nothing is kept without the node showing it, and the next `RemoveFile` after reconnecting releases it. - I didn't add an automatic retry for user removals. The user asked for this removal and can see that it failed, so a background retry would remove a file they may have decided to keep after all. Chat attachments are different, because nobody is watching that release. They already go through the durable `file_pin_operations` queue, which retries on reconnect. The test is `TestAFileThatCannotBeLetGoStaysListed` in `api/local_files_test.go`. It covers four cases: 1. A store that answers `UNAVAILABLE` makes the removal fail, and the file stays listed next to an unrelated one. 2. After "reconnecting", the same removal succeeds and only that file goes. 3. When re-listing also fails, the error says so. 4. When the list itself cannot remove the entry, the store is never asked to let it go. The docs in `desktop-backend.md` describe the behaviour. Could you take another look?

excho0 approved these changes

2026-10-19 14:28:42 +00:00

excho0 left a comment

Reviewed the updated 13e1b41 change. It restores the exact catalogue entry when remote Unpin fails, preserves the store status code, and covers retry after reconnect plus list/restore failures. The failure path I raised is addressed; CI is still running, so this is code approval only. /cc @rvnminers-A-and-N

Reviewed the updated 13e1b41 change. It restores the exact catalogue entry when remote Unpin fails, preserves the store status code, and covers retry after reconnect plus list/restore failures. The failure path I raised is addressed; CI is still running, so this is code approval only. /cc @rvnminers-A-and-N

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Sisyphus/Sisyphus#505