v3: feature: verdi collab - #7516
Open
khsrali wants to merge 42 commits into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesCollaboration provenance sharing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7516 +/- ##
==========================================
+ Coverage 80.67% 81.48% +0.81%
==========================================
Files 581 589 +8
Lines 47068 49424 +2356
==========================================
+ Hits 37967 40267 +2300
- Misses 9101 9157 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
khsrali
force-pushed
the
collab
branch
2 times, most recently
from
August 11, 2026 08:09
0ac7d37 to
1676f17
Compare
`verdi node rehash` recomputed each hash through `node.base.caching.rehash()`, which writes the extras through the regular path and so fires the `mtime` `onupdate`. A hash describes a node, so recomputing it modifies nothing, yet every rehashed node came out looking edited today — to `verdi process list --past-days`, to an incremental `verdi profile dump`, and to anything else that reads `mtime` as "when this was last changed". Write the hashes with a bulk update that passes the existing `mtime` explicitly, batched, so the timestamp survives the rehash. The collab series that follows reads `mtime` to decide what a peer is owed, which is what makes this a prerequisite rather than a tidy-up.
First step of `verdi collab`: profiles on a trusted private network sharing one provenance graph, each serving an HTTP endpoint the others pull from and push to. Two stores. The `collab.*` profile options hold what a person configures — whether the profile takes part, the shared token, the address to serve on. The sidecar under `~/.aiida/collab/<profile uuid>.json` holds what syncing produces: the per-peer cursors and the log of completed transfers. It is a separate file because `config.json` is re-read on every `verdi` invocation and rewritten on every store, while this grows with every sync. Keyed by profile UUID rather than name, so a profile recreated under the name of a deleted one does not inherit its cursors, and `Config.delete_profile` takes the state with it.
The types both sides of a collab exchange, in one module that neither the client nor the server owns: the handshake a peer answers with, the manifest of a delta and the diff of it the receiver asks for, the offer that names the export instant, and the errors a request can fail with. Under them, plain HTTP on a private-network address, with the shared token as a bearer credential — the overlay provides the encryption, which is why binding all interfaces is refused rather than merely discouraged. A stdlib `ThreadingHTTPServer` and a `requests` client, so no new dependency. Both directions resume: a download honours `Range`, an upload honours `Content-Range` and appends to a staging file named by its checksum, so a transfer interrupted at N bytes costs N bytes, not the whole delta. Addresses are family-neutral, since the overlays this is meant for hand out IPv6 as readily as IPv4.
What a sender owes a receiver is the sealed processes it has produced since the receiver's cursor, plus what it imported from anyone since then — the event union is what lets provenance relay through a third profile instead of being dropped for having an old `mtime` — closed over provenance, minus the UUIDs the receiver declares it already holds. The export therefore takes the receiver's claim and the subset it asked for and writes only those nodes, which is what keeps a reused heavy input from travelling twice. The archive that comes out is deliberately not provenance-closed: its links may name nodes only the receiver holds.
A collab delta is written with links that point outside it, at nodes the receiving profile was declared to already hold. `verdi archive import` would import such a file happily and silently drop every one of those links. Refuse it by name, pointing at the command that knows how to resolve them. `import_archive` itself stays permissive — collab's own import path calls it on exactly these archives.
The receiver's half of a thin transfer. Which nodes travel is settled before any archive exists: the sender offers its UUID manifest, which is cheap, and the receiver answers with the subset it lacks, so only that subset is written. What arrives therefore has links that point out of it. One whose endpoint is absent from the archive resolves to the local node of that UUID. An endpoint present in neither aborts the import before anything lands, naming the node, and the next negotiation — whose diff now includes the hole — recovers it. A link that would violate an invariant against the local graph, a second CREATE parent or a second caller, aborts the same way rather than being written. The boundary links are journalled to the state file before the import and cleared once written, because the import commits its own transaction and a crash between the two would otherwise lose them.
Wires the transport to the sync core: negotiate a delta for the cursor and claim a peer presents, cache the export it produced so a resumed download serves identical bytes, and import what a peer uploaded. Accepting a push is opt-in and read per request, so revoking it takes effect at once. While an import holds the lock the handshake answers busy before anything is exported or uploaded, which is what makes concurrent fan-in serialize with no redundant bytes. On SQLite, single-writer, the workers are stopped around the import and restarted even if it raises.
A circus watcher inside the existing daemon, invoking a hidden `verdi daemon collab-endpoint`, mirroring how the message broker is supervised. Added only when the profile takes part in a collab, with its own log file. No user-facing command to start or stop it: the endpoint's lifetime is the daemon's. The watcher list is lifted out of `_start_daemon` into `_create_watchers`, so what the daemon would launch can be asserted without launching it.
One code carries everything a joiner needs — the collab UUID, the issuing member's URL and the token — and any member can mint one. Three identities, three lifetimes: the token authenticates, the profile UUID names a member permanently, and the collab UUID names the collab itself, so a contact belonging to a different collab is refused rather than spliced in. Membership is a roster keyed by profile UUID; nicknames are local display names only, deduplicated on collision, and are what the commands and their shell completion accept. `verdi collab init` either founds a collab or joins one, and refuses to complete without an address to serve on, since an enabled profile whose endpoint cannot bind would crash-loop it. The code is minted here but printed by `verdi status`, which gains its collab section later in this series.
What a collab shares beyond provenance nodes — whether extras replicate, whether curated group membership travels — is one decision taken once by whoever founds it, and never changed by anyone. Changing the terms means re-founding the collab. Stored as a single dict-typed option, which `verdi config set` cannot write, so immutability costs no machinery and the pair is never offered as a knob with no legitimate change path. The join code carries it, so the consent moment happens before anything is created, which is the last point at which a newcomer can still decline the terms.
The roster travels. Every handshake and every delta request carries the entries a peer knows, and the merge adopts what it does not have, so a newcomer admitted by any member is known to all of them after one round instead of every machine hand-maintaining every other machine's URL. Addresses are versioned by their owner, and only the owner raises its own stamp. That is what makes "whose information is newer" a local fact: a stale URL cannot gossip its way back over a fresher one, and a member whose address changed is corrected everywhere its new stamp reaches. `verdi collab peer` lists what this profile holds, and corrects it by hand where gossip cannot.
Both take peer nicknames and default to every peer, both negotiate first and prompt with the exact node count and byte size before anything travels, and `--force` skips the prompt. Until the prompt is answered the only writes are cache artifacts, so declining leaves both profiles byte-identical. `--dry-run` stops after the manifest diff. An offline, busy or refusing peer is warned about and skipped rather than failing the loop, and so is one whose handshake declares a policy other than this profile's — which can only mean a hand-edited configuration. Pull on SQLite with running workers aborts unless `--pause-my-daemon`, since the storage takes one writer.
The handshake reports a peer's storage backend as well as its archive format, but only the archive format is an interchange contract: the delta travels as an archive, and storage is a local concern, so a collab mixing PostgreSQL and SQLite profiles is fine. Gate on the archive schema alone, at the handshake, before any export. The message is aimed at whoever has to act: pulling from a peer writing a newer format tells you to upgrade, pushing to one reading an older format asks you to have them upgrade. In the direction that can work, an older peer's delta is migrated forward rather than refused.
`verdi collab log` shows what this profile has pulled and pushed, when, with whom and how much. Pushes start being recorded here; until now the log held only what came in. The log is also read by every negotiation, so it cannot grow without bound: past a threshold all but the newest half fold into one synthetic event per direction, at the horizon, carrying the union of their UUIDs. Every distortion this can introduce is over-statement — an inflated claim or manifest, which is metadata the diff then drops — and never omission, which would lose relayed provenance.
The token authenticates every request, so unless it can change a leak has no remedy and a member no exclusion. `verdi collab rotate` mints a new one and prints a fresh code; `verdi collab rekey` adopts one. The collab UUID is untouched — rotation replaces the key, not the identity. The new code travels out of band, human to human, because the in-band channel is authenticated by the very token being retired. The signal sent to reachable peers is advisory only and never triggers an action, for the same reason. Rotation rests the whole roster. A member returning under the current token is recognized from history and resumes at its existing cursor; one that never rekeys stays dormant, invisible to sync and to status, its record kept.
One section on a collab-enabled profile: each active peer with its address and whether it answers, the policy — which `verdi config set` cannot write, the option being immutable — the last sync, any signalled rotation, and the join code, since any member can admit a newcomer. Peers are probed concurrently with a short timeout, so one unreachable machine costs the timeout and not the sum of them. Profiles without a collab print nothing. This is what makes a collab reachable end to end: until now the join code could be minted but never shown.
Deleting a node from one profile of a collab does not delete it anywhere else — each profile owns what it holds. But a peer that still has it would deliver it back on the next sync, so a deletion records the UUIDs it removed, and they ride in the claim as nodes already held. Recorded after the deletion succeeds, from UUIDs read before the rows were gone. `verdi collab pull --include-deleted` is the way back: it rewinds the cursor for that pull and drops the tombstones it re-imports.
Under a `sync` policy, extras become the one thing that can change after it has travelled. Negotiation gains a symmetric exchange of `(uuid, mtime)` for shared nodes edited since the cursor, and where the peer's node is newer its whole extras dict replaces the local one — a snapshot, so deletions propagate as absence. Two invariants keep it from oscillating and from trampling: a refresh write preserves the sender's `mtime`, so the receiver never becomes the newest side and echoes back; and keys starting with `_` never travel in either direction, which keeps the caching extras local and gives users a private namespace that survives an incoming snapshot.
Under a `grow` policy the user-curated groups of a delta's nodes travel with them, and membership already held is offered separately — a `GROUP_NODE` row carries no timestamp, so "what changed since T" has no local answer, and curation is journalled as it happens instead. Additions only: removing a node from a group does not remove it anywhere else. Groups AiiDA generates for itself never travel, since they describe how provenance arrived in one profile rather than what a person curated.
A pulled calculation is a cache miss locally, because its hash names the peer's computer. Declaring two computers equivalent lets the hash be recomputed against the local one, so an identical submission here hits the peer's result instead of running again. Opt-in and reversible: without a mapping nothing is remapped, and the remap touches the hash extra alone — UUID, attributes and repository content are untouched.
One semaphore over the expensive concurrent work of the endpoint — incoming push sessions and served pull negotiations together. Beyond the cap a peer is answered busy and retries. Slots expire after idleness, so a holder that crashed cannot wedge the endpoint.
The how-to: the trust model and what it assumes of the network, setting up and joining, the membership lifecycle, day-to-day pulling and pushing, and the things that surprise — deletion never propagates, extras are the one mutable surface, group membership rides only with curation, a cache hit can hand you a `RemoteData` on someone else's cluster, and the timestamps a sync writes. Also ignores the `.aep/` directory in which the design notes of this feature were kept.
The unit tests drive one profile against a stub. This drives three real profiles against each other over live HTTP endpoints — the shape a collab actually has, and the smallest one in which relay through a third member, gossip and divergence can happen at all. One harness builds the members and wires them pairwise, and a fault injector breaks transfers where they can break. Six files, one claim each: that pairwise syncs converge and nothing echoes back; which divergences between copies are the design and which would be defects; who is in the collab, and how they join, move, rotate, rekey and are refused; what survives a transfer that breaks mid-flight; what an interrupted sync leaves for the next one to find; and what a collab of mixed aiida-core versions rests on. `test_pull_push_end_to_end` moves out of the command tests, where it was the one test standing in for all of this. The tolerant `data.get(...)` reads in `CollabState.read` go with it. Every state file the collab writes carries every key, so the defaults could not have been reached by anything but a truncated write, which they would have masked. Also adds a `CLAUDE.md` deferring to `AGENTS.md`, with the conventions this branch was written under.
The collab UUID travelled as a per-route JSON body field, so the routes a push carries its payload over could not check it: `POST /missing`, the upload probe and `POST /import` carried no such field, and `PUT /upload` carries no body at all. Only the handshake stood in front of them, and a client is free not to send one, so a holder of a token shared too widely reached the upload and the import directly and landed foreign provenance in a profile of another collab. It now travels as the `X-Collab-UUID` request header and is checked in `_dispatch` right after the token and before routing, which covers the upload routes and every route added after this one by construction. The refusal closes the connection and says so, because the body of a refused upload is never read. `GET /info` stays exempt: it is the one route whose purpose is to say which collab this is, and the precise refusal that names both values is assembled from its answer.
`config.json` had two writers and no cross-process lock. `verdi collab rotate` and `verdi collab rekey` write `collab.token` into the same file the daemon's endpoint rewrites when it merges gossiped roster entries into `collab.peers`. Both re-read the file before writing, but neither held a lock across the read and the write, and `Config.store` serializes the whole document: a merge that reads before a rotation and stores after it writes the old token back, while `rotate` has already printed a join code nobody holds. `tools/collab/config.mutate_config` is the mirror of `CollabState.mutate` for the configuration: hold the collab lock on a sidecar of `config.json`, yield the file as it is on disk, store it on exit. All nine collab write sites go through it, and `stored_config` — whose job was "re-read before writing" — goes with them. Only a document that changed is written back, so a dry run that announces itself without raising its stamp still leaves the file as it found it. `self_entry(bump=True)` stops storing. It is the one site that writes from inside another one's transaction, and it used to store the in-process configuration nobody had re-read: a `rekey` by a member that had moved while it was dormant reverted every entry the endpoint merged in between, and every other profile's options with them. It now mutates the configuration it is handed and lets the enclosing `mutate_config` store. `state._exclusive_lock` becomes `exclusive_lock`, taking the lock path, so the state file, the import lock and the configuration share one lock implementation rather than three copies of the same `flock` dance. The endpoint's roster mutex goes with it: `flock` already serializes its threads. Two things this does not do, both recorded rather than hidden: a plain `verdi config set` still takes no lock, and Windows has no `fcntl`, so the configuration lock inherits the gap the state lock documents.
…es staged `collab.accept_push` is off by default, so a profile has to opt in to being written to. The refusal came at the last possible moment: `POST /handshake`, the first request of a push, did not consult the option at all, and `import_staged` at the very end was the only place that enforced it. A pusher whose consent was withdrawn between reading `GET /info` and pushing computed a delta, exported it to a real archive and uploaded every byte of it before finding out. Worse, the refusal fell through to `_dispatch`'s generic handler and was answered 500 with a logged traceback, so a policy decision read as a broken endpoint to whoever kept the daemon log. The handshake now reads the option — from the file, per request, as `local_info` and `import_staged` already do — and refuses before it merges a roster, grants a slot or reads any state, so nothing is exported and no byte travels. The refusal is a `PushRefused`, a class of its own beside `EndpointBusy`, which `_dispatch` answers 403 with its message; the stdlib `PermissionError` it reads as would have swallowed an `EACCES` on a file the daemon owns and reported a broken deployment as a policy decision, logged nowhere. The import keeps its own refusal: a peer that skips the handshake and goes straight to the upload routes would otherwise land its push. The upload routes stay unchecked. A cooperating peer never reaches them and a deliberate one is refused at both ends, so a per-chunk check would only cost work on a path already refused twice; it is no security boundary either, since every member holds the token and can fill the same disk through legitimate pushes. What such a peer does leave behind is bounded by a sweep rather than a check. A staged upload was removed only when its import succeeded, failed its checksum or was refused for good, so every other outcome — the pusher dies, the import raises, the peer never retries — left the file in `~/.aiida/collab/<profile-uuid>/staging/` forever, one per failed push, with nothing reporting the growth. The endpoint now sweeps that directory of files older than `STAGING_MAX_AGE` at startup, next to the `delta-*.aiida` sweep it already did. Seven days is measured against what the file is for: a stash exists so that a retrying pusher transfers zero bytes, and one that has not retried in a week is not coming back with them. Sweeping a file that would have been retried costs a re-upload, never correctness. No size cap and no quota: that is a configuration knob with no second caller. One consequence is recorded rather than hidden — a profile that does not accept pushes no longer reads the roster a pushing peer gossips with its handshake, so a member that moved or rekeyed is learned about there on a pull negotiation or a join instead, which is the route `verdi collab rekey` takes anyway.
`collab.max_concurrency` caps how many peers an endpoint serves at once. The
push slot was keyed by the pusher, `push:{requester}`, but the pull slot was
keyed by the shape of the request, `pull:{hash(cursor, claim)}` — and every
newcomer of a collab presents the very same empty cursor and claim.
Two of them therefore counted as one session against the cap, so an endpoint
set to serve two peers would let a third in. Worse, they shared a slot: when
the first finished its download, the release freed the slot the second was
still being served under, and that peer either silently re-acquired on its next
request or was refused if the slots had filled meanwhile.
The pull slot is now `pull:{requester}` as well. The requester reaches the
endpoint as an `X-Collab-Peer` header — set once on the client's session, read
once in `_dispatch`, handed to the handler — following the pattern the collab
UUID established, and for the same reason: the routes that need it include the
upload, which streams raw bytes and has no body to carry it. The value is the
peer's own profile UUID, which the push handshake already sends and the pull
already gossips. It names a session and authorizes nothing — the token
authorizes and the collab UUID confines, both before it is read — so a request
without it is served under one anonymous session rather than refused. Turning a
client that omits it into a lockout would be the worse failure.
Keyed by the peer, the map from an exported delta to the slot behind it says
nothing the key does not, so it goes, and with it the eviction path that
dropped its entries without releasing them. `release_delta(delta_id)` becomes
`release(requester)`: what ends is a peer's session here.
The cap remains approximate, as it was: a slot is refreshed per request, so a
single transfer longer than `SLOT_IDLE_SECONDS` can still be reclaimed under
its holder.
A serving slot was only ever given back by a transfer that finished: the import committed it, or the download completed it. A negotiation that never reached either had no release path at all, and the commonest reason to negotiate is not to transfer. `verdi collab pull --dry-run` negotiates a manifest, reports what it would fetch, and stops. So does `--dry-run` on the push, after its handshake. So does declining the confirmation prompt, which is offered only after the manifest was already negotiated. With the documented default of two peers served at once, two members running a dry run left the endpoint answering everybody else busy for ten minutes, with nothing on either side reporting why. A negotiation now ends: `DELETE /collab/v1/session` frees the slots of the peer that asks, named by the same `X-Collab-Peer` header its requests carry. Every path that walks away from a granted slot says so — both dry runs, both declined prompts, a push that finds the peer up to date, a transfer that failed, and an import the peer refused before it ran, whose own release belongs to the import that never happened. Best effort, and briefly so. `SLOT_IDLE_SECONDS` remains the backstop for a client that dies, and the two answer different failures: the expiry covers the client that cannot speak, the explicit end covers the one that simply finished. A peer that cannot be reached to be told is logged at debug and left to the expiry — this must never cost a user their command, nor stall it a second time waiting to be polite about it, which is why the release has its own short timeout rather than the full one of a transfer. An older peer answers 501 to the new route, which is swallowed like any other failure to be heard: it simply keeps the behaviour this commit replaces.
`verdi collab pull` and `verdi collab push` loop over the selected peers and skip every unusable answer with a warning: offline, 401 after a rotation, busy, an archive format this profile cannot read, a policy that does not match. Three failures were fatal instead. An `IntegrityError` from a boundary link the receiver cannot resolve, a `ConfigurationError` from a stale `collab.computer_map` and every failure of a push import all reached `echo_critical`, which exits: pull five peers, have the second serve a delta that cannot land, and peers three to five are never contacted, with the partial progress visible only in `verdi collab log` — against what `collaborate.rst` promises. Neither message named the peer it came from, which with several peers is the only thing that says which one to look at. The exceptions are now separated by what they are about. A delta that cannot land is one peer's answer: the boundary refusal, the archive reader's errors — `CorruptStorage` and its siblings are `ConfigurationError`s, which is how an unreadable delta was already reaching that catch — and every failure of a push import join the peers that are warned about and skipped, by name. A stale `collab.computer_map` is about this profile and would refuse every peer's delta identically, so `resolve_computer_map` runs once before the loop and aborts there, having contacted nobody and transferred nothing. The exit code still tells the two apart. Both loops collect the peers whose transfer started and did not land and exit non-zero at the end naming them, while a peer that never started one — offline, busy, refusing, mismatched, skewed — leaves the exit code at zero. That distinction is a judgement call and one line from the alternative, of exiting zero always and leaving failures to `verdi collab log`. The two tests that asserted the defect are inverted rather than replaced. The injectors that plant a boundary link or corrupt an export now fire on the next delta only, so a round contains exactly one unusable peer and the peers on either side of it are what show the loop carrying on.
A push that fails after its bytes have landed keeps them and retries them verbatim, which is what lets the upload resolve to what the peer already staged. The retry renegotiated the group memberships but left `refresh = []`, and its import still advanced the receiver's cursor to the stashed instant. `refresh_offer` bounds its offer by that cursor and the edit the failed push had already offered is older than the instant, so it was never offered again — not by the retry, not by any later push, and no delta could carry it either, the node having been shared long before. Under `extras_mode: sync` the user saw a successful retry and lost part of what it reported. The identical defect for memberships was fixed eleven lines below, with the reason written out in the comment above it; that sentence was true word for word of extras. The retry now computes `refresh_offer` against the handshake cursor and takes the snapshots the receiver asks for, in the same `diff_manifest` call that carries the curations. What deliberately does not change is the delta: the bytes are the ones the peer has staged and re-cutting them would defeat the resumption the stash exists for. Only the metadata travelling beside them is negotiated again.
Both loops computed from a `CollabState` read at the wrong moment. The state file is a snapshot, and what a journal entry means depends entirely on the instant it is compared against. The push read it above `compute_delta`, which takes the export instant the receiver's cursor will advance to. A curation or an extras refresh another process journalled while the delta was being cut — a traversal and an export, not two adjacent statements — was therefore missing from the offer and behind the receiver's cursor the moment the import landed, which is forever. Both branches now read after their instant. Over-stating an offer is free, since the manifest diff and the mtime comparison drop whatever the receiver already holds; omitting one is permanent. The pull handed `import_delta` the copy taken at the top of the peer loop, before a handshake, a negotiation and a download. `state.tombstones` is what the import honours, so a `verdi node delete` run while a delta was on the wire was silently undone: the tombstone reached the state file, the in-flight import read from before it, and the node was imported straight back. It now re-reads inside `import_lock`, as the endpoint has always done. The pre-download copy keeps its job — it is what the cursor and the claim of the negotiation were computed from, and those must describe the moment the negotiation happened. The deletion is driven from inside the negotiation rather than the download it races in practice: the archive holds exactly the nodes the receiver reported missing when the manifest was diffed, so a deletion later than that has nothing to be undone by, and a test written at the download would pass either way.
`verdi status` printed the join code of the collab unconditionally. The code is `base64(json(...))` — encoding, not encryption, and `JoinCode.decode` is a public method of the same module — and it embeds `collab.token`, the shared secret every request of the collab is authenticated with. `verdi status` is the command users are routinely asked to paste into bug reports, chat threads and GitHub issues; anyone reading one held the key to that collab and, inside the trusted network it lives on, read access to every member that had not rotated. The setup section of the how-to told users to read the code from there, so this was the documented workflow rather than an accident of usage. The reasoning that put it there stands: any member can admit a newcomer, so every member has to be able to show the code, and a member that has to rekey after a rotation obtains it the same way. What was wrong is only that obtaining it was not an act. `verdi collab link` prints it, and `verdi status` keeps the line with the code withheld — so a member still sees that a collab is configured and where the code comes from. Named `link` and not `code` because `verdi code` is an established top-level command for something else entirely, and because the string is the thing you hand somebody so they can join. Nothing about the code itself changes: same encoding, still not a secret in transit, still printed in full when asked for. The `verdi status` test asserts the absence by trying to decode every word of the output rather than by matching the redaction text, which would keep passing if a later change printed the code in another shape.
Sending only the tombstones recorded at or after the cursor a peer presents was proposed as a way to stop the claim growing without bound. The reasoning was that the sender's seed filter is bounded by that same cursor, so a node behind it cannot be a seed of anything the sender will offer. It is not shippable, because the start set of a delta is not the seeds: it is the seeds united with what the sender imported since the cursor (`sync.py:147`), which no mtime bounds, and the seed filter itself is mtime and nothing else, so any write on the sender — setting an extra of its own is enough — lifts a long-deleted process back over the cursor. Verified end to end on three members over the wire before being reduced to this test: a node Alice deleted, that Bob later took from Carol, comes back into Bob's delta the moment Alice's claim stops naming it. Alice does not hold it, so she asks for it, it travels, and the import throws it away — an enlarged delta, a wasted transfer and an offered-then-refused import, which is everything the bound was required not to cost. So nothing in `src/` changes. What ships is the test that says why, over both re-entry paths, and a note in the how-to stating the residual as it actually is: the tombstone set is unbounded in the state file and on the wire, and only deleting the profile clears it. The follow-up worth having is not a smaller claim but no tombstones in the claim at all — filtering them at the manifest diff instead, where the thin delta already makes it free — which touches regions this phase does not own.
A delta is computed once per `(cursor, claim)` and cached; `_stale` decides when to recompute. A withheld seed — a sealed process the export cannot write because its provenance reaches a process that is still running — broke that permanently. `compute_delta` pulls the export instant back to such a seed's mtime, so the seed stays within reach of the next computation, and `_stale` then asked whether any sealed process had `mtime >= instant`. The withheld seed satisfies that by construction. Every `negotiate_delta` and every `request_delta` therefore re-ran the computation — two full graph traversals — for a profile that had gained nothing. A workchain that excepts while a child it called never seals is enough: the daemon was killed, the process is stuck, and from then on the requester's cursor never passes that mtime, every pull recomputes from an ever-older cursor over an ever-larger start set, and the claim grows with it. `Delta` now carries the instant the computation was taken at beside the export instant, and staleness is measured against that one: a fact about content gained, not about a timestamp the withholding itself moves. The export instant keeps its meaning and every consumer of it — the archive, the receiver's cursor, the superseded-archive check in `request_delta` — reads the same value as before, and the seed filter stays inclusive for the reason its comment gives. The computation instant is required rather than defaulted. A default would supply "just now" to whatever built a `Delta` without thinking about it, which reads as freshly computed — the failure this commit exists to remove.
Nothing anywhere said that a profile was holding provenance back. A sealed process whose own provenance reaches a process that is still running cannot be exported — the export refuses to write an unsealed process, and the rules that pull in called processes cannot be turned off — so it is left out of every delta until that child seals. When the child never seals, because the daemon was killed or the process is stuck, the subgraph stops travelling for good and the only symptom is a peer that never receives it. To that peer it looks like provenance that was simply never produced. `verdi status` now names it: how many such processes there are and how old the oldest is, and nothing at all when there are none. It is worth saying here precisely because the cache fix removed the visible symptom — a sync that quietly got slower every day — and left the condition silent. `withheld_seeds` asks the question the same way the delta computation does, sharing the reverse walk that answers which nodes reach an unsealed process, so the row cannot drift from what a delta would actually withhold. `_unsealed_pks` stops answering by complement. Taking every process in the profile and subtracting the sealed ones was cheap while the only caller bounded it to a delta's closure, and is 1.77 s and ~25 MB of transient sets on 200 000 processes when the caller is a status line. It asks for the absence of the attribute instead: `seal()` is the only writer of the key and only ever writes `True`, so a process without it is exactly an unsealed one — 0.024 s where the complement takes 0.401 s, and the same ids on both PostgreSQL and SQLite. The bounded caller goes the same way rather than keeping the old form beside the new one. The section holds the storage open until it has finished with it, which is why `verdi status` closes the backend at the end of the command rather than as soon as it has printed the storage row.
…and a download that has to prove it arrived Three things the collab additions left behind, none of them visible on the versions a developer runs, which is why all three survived until CI. `ProfileOptionsSchema.collab__policy` is annotated with `CollabPolicy`, declared as a `typing.TypedDict`. Pydantic refuses to build a schema from one below Python 3.12, where the runtime does not preserve enough of the declaration for it to introspect. Every 3.10 job therefore died on the first `verdi` invocation, before a single test ran, and the Docker image — Python 3.10.13 — timed out waiting on a container whose entrypoint crashed the same way. The declaration moves to `typing_extensions.TypedDict`, which carries what pydantic needs on every version we support. The other typed dictionaries in the module follow the same import rather than leaving the next one to remember which of the two it wants. `Config.filepaths` grew a `collab` entry for the log the collab endpoint writes, and `verdi bug-report` still named the four log types it knew about. A profile running an endpoint therefore produced a bug report without the one log that would say why the endpoint failed — the case the command exists for. `test_bug_report_command` derives its expectation from `filepaths` itself, so it said so as soon as the entry appeared. `download_delta` left it to `requests` to notice a body that stops early. Whether it does is a property of the HTTP stack rather than of the transfer: urllib3 enforces `Content-Length` only from 2.0 on, and below it a dropped connection is a short read that `iter_content` returns as though it were the whole delta. The download then reported success and the truncated archive went to the importer, which is a good deal worse than the interruption it came from — this one was caught because a truncated zip is unreadable, and a truncation landing on a boundary the reader accepts would not be. The client now measures what arrived against what the peer declared and raises when they differ, which is the path a dropped download already took: the peer is skipped for this round, its partial file kept, and the next pull resumes from it. Two assertions about that partial file went with it. They pinned the size at exactly one chunk, which is not the design's promise but urllib3 2.x's buffering — under 1.26 the stack yields the short tail as well, so the prefix is longer. They now assert what the resumption actually needs: that a prefix of the delta survived, no longer than what the peer managed to serve.
A pulled calculation brings the computer it ran on, and that computer landed under the label its owner gave it — indistinguishable in `verdi computer list` from the machines of the profile itself. Nothing told a user that `lumi` was a collaborator's cluster they have no account on, and nothing told them which label to write on the left of a `collab map-computer` pair, which is the one place the distinction has to be made: name the wrong half and the remapped hashes are written onto their own calculations. Where two collaborators both ran a `lumi`, the importer resolved the clash on its own terms and the peer's machine became `lumi (Imported #0)` — a name for an accident rather than for a computer. Every computer an import creates is now relabelled `<label>@collab`. Writing the marker onto a label that already carries it is a no-op, so a machine that travels A → B → C is `lumi@collab` on both B and C, and one that circles back to its owner is matched by UUID and creates no second row: the originator keeps its plain `lumi`, everybody else agrees on one name for one machine, and the mapping — and the cache hits it exists for — cannot be split across two rows for one physical cluster. Clashes are deduplicated with the index before the suffix, `lumi-2@collab` and never `lumi@collab-2`, because the next hop recognizes a marked label by its ending and a displaced marker would be appended to again on every hop. Relabelling is safe for caching only because a hash is computed from the UUID of the computer and never from its label. That is what `test_relabelling_a_computer_leaves_the_hash_of_its_calculations_alone` exists to hold: the day it stops being true, every mapped calculation in every collab silently becomes a cache miss. Which computers an import created is a question only the state from before it can answer, and `import_archive` commits its own transaction, so the answer is journalled as `pending_computers` before the import and cleared once the marking is done — the same window, and the same treatment, as the boundary links one field above. A crash in between leaves the entry for the next import from any peer to finish. Without it a machine that arrived during a crashed pull would sit under a plain label for good, since nothing else ever revisits a computer the profile already holds. The marker is neutral rather than the peer's name. A nickname for a peer is local, never travels, and on a relay would name the member the machine came through rather than the one that runs it. It leaves the originator seeing a different label from everyone else, which is deliberate: renaming a user's own computer to record that somebody else has now heard of it is not this feature's business.
`_remap_hashes` finds the calculations to rewrite by querying *this* profile for the peer label of each pair, so the peer half has always had to name a computer that had already arrived here. `resolve_computer_map` checked only the local half. A mapping naming a machine that had never been pulled — a typo, a label the collaborator uses that this profile has not seen, or simply a mapping declared too early — matched nothing, rewrote nothing, and reported `2 computer mapping(s) configured, the mapped hash was written onto 0 calculation(s)`: a success line for a mapping that could not work, no cache hits, and nothing anywhere to say why. At pull time it was quieter still, the same dead pair being resolved on every import for as long as it stayed in the option. Both halves are now checked against the computers this profile holds, and a pair whose halves resolve to the same computer is refused as well — it can only be a mistake, and honouring it would write remapped hashes onto this profile's own calculations. A call is refused whole when any pair fails, and every unusable pair is named in one message: `collab.computer_map` is a single option, and applying the good half of it leaves a set of declared equivalences harder to reason about than none. The refusal lists the peer computers that did arrive, which is what makes it actionable — the label to map from is the marked one. `verdi collab init --map-computer` goes with it. Both halves must be held, and at init time nothing has arrived, so the flag could only ever record a mapping that was inert until the first pull. Waiting costs nothing, since `map-computer` applies a mapping to the calculations already pulled; the report now names the pairs this call declared rather than counting the ones the option accumulated, and says plainly when it rewrote nothing and why. What this leaves undone: the map is keyed by a label, and a label is mutable on both sides. A peer that renames its computer, or a user who renames their own, leaves a pair naming nothing — caught at the next pull, which aborts before contacting anyone, and answered with a 500 by an endpoint that resolved the option at startup. Keying by computer UUID is the lever, at the cost of an option nobody can write by hand. Nor does the check ask whose computer is whose: mapping one of this profile's own machines onto another of them is still accepted, because telling them apart needs a heuristic about provenance this feature deliberately does not have. The `@collab` marker makes the right answer visible instead of guessing at it.
Two collaborators looking at the same node see two different numbers, because the primary key is assigned by the database of whichever profile the node landed in. Nothing said so, and the whole of the rest of AiiDA teaches a user to identify a node by its PK: `verdi node show 1234` in a message to a collaborator points at some other node on their machine, or at nothing. The UUID is what to quote, and a unique prefix of it resolves the same way the full one does, which is short enough to say out loud. The clause on why is there so nobody re-opens it: preserving PKs across profiles would mean partitioning a 4-byte id space between the members, which survives neither their number nor SQLite's `max(rowid) + 1` allocator. Its own commit rather than riding with the two that surround it: it was decided alongside them and would otherwise have been forgotten, but it describes neither the naming of imported computers nor the guard on the computer map, and folding it into either would make that commit's message wider than its subject line claims.
khsrali
marked this pull request as ready for review
August 11, 2026 13:15
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (16)
tests/cmdline/commands/test_status.py (1)
521-554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace loopback sockets with a mocked client.
This command-level test opens real network sockets and waits for I/O timeouts. Mock
CollabClientinstead. Record the supplied timeout and coordinate mockedinfo()calls to verify concurrent probing without network timing variability.Based on learnings: “Mock anything slow or external, including network, filesystem access beyond a temporary directory, databases, subprocesses, time, and third-party APIs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cmdline/commands/test_status.py` around lines 521 - 554, Replace the real loopback sockets and elapsed-time assertion in test_status_collab_probe_timeout with a mocked CollabClient. Have the mock record each supplied probe timeout and coordinate its info() calls with synchronization primitives to verify probes execute concurrently; retain assertions for six offline peers, the reachability summary, and the expected timeout value.Source: Learnings
src/aiida/manage/configuration/settings.py (1)
97-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse Sphinx-style docstrings for new public callables.
src/aiida/manage/configuration/settings.py#L97-L100: Add a:return:directive toAiiDAConfigPathResolver.collab_dir.src/aiida/tools/collab/config.py#L47-L51: Convertis_enabledand the remaining new public callable docstrings to Sphinx directives.src/aiida/tools/collab/protocol.py#L107-L121: ConvertJoinCode.encode,JoinCode.decode, and the remaining new public callable docstrings to Sphinx directives.src/aiida/tools/collab/state.py#L142-L152: ConvertCollabStatepublic callable docstrings to Sphinx directives.As per coding guidelines, “Use Sphinx-style docstrings (
:param:,:return:,:raises:), with types written in annotations rather than docstrings.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/manage/configuration/settings.py` around lines 97 - 100, Update the new public callable docstrings to use Sphinx directives and rely on annotations for types: add a :return: directive to AiiDAConfigPathResolver.collab_dir in src/aiida/manage/configuration/settings.py (97-100); convert is_enabled and the remaining new public callable docstrings in src/aiida/tools/collab/config.py (47-51); convert JoinCode.encode, JoinCode.decode, and the remaining new public callable docstrings in src/aiida/tools/collab/protocol.py (107-121); and convert CollabState public callable docstrings in src/aiida/tools/collab/state.py (142-152).Source: Coding guidelines
tests/tools/collab/test_e2e_failures.py (2)
238-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not assert on a third-party error message.
'not a folder, zip or tar file'comes from the archive reader dependency, not from aiida-core. An upgrade of that dependency breaks this assertion for a reason unrelated to the behavior under test. The stable parts of this claim are already asserted: the peer is named, the run exits non-zero, and nothing landed. Match on the aiida-core wrapper message instead, or drop this half of the assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_e2e_failures.py` at line 238, Update the failure assertion in the pull branch of the end-to-end failure test to avoid matching the third-party archive-reader text; assert the stable aiida-core wrapper message instead, or remove that branch while retaining checks for the named peer, non-zero exit, and no landed provenance.
322-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the development-phase artifacts from these docstrings. Both docstrings open with an
EXPECTED (phase N)marker from the development sequence of this pull request, and one points at a planning file that will not exist in the merged repository. The docstrings should state the behavior under test only.
tests/tools/collab/test_e2e_failures.py#L322-L327: drop theEXPECTED (phase 3):prefix and start the summary line with the behavior, for example "a profile that has not opted in to being written to is skipped, and still serves pulls".tests/tools/collab/test_e2e_failures.py#L465-L473: drop theEXPECTED (phase 9):prefix and remove theSee ``phase-15/deferred.md``.reference at Line 469; keep the explanation that the expiry is pinned rather than the cap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_e2e_failures.py` around lines 322 - 327, Remove development-phase references from both test docstrings: at tests/tools/collab/test_e2e_failures.py:322-327, remove the “EXPECTED (phase 3):” prefix while preserving the behavior description; at tests/tools/collab/test_e2e_failures.py:465-473, remove the “EXPECTED (phase 9):” prefix and the “See phase-15/deferred.md.” reference, while keeping the explanation that expiry is pinned rather than the cap.tests/tools/collab/test_endpoint.py (1)
137-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the
match=patterns flagged by Ruff.Ruff reports RUF043 on these three
match='collab.accept_push'patterns. The dots are regex metacharacters. Usere.escapeor a raw pattern to silence the rule and pin the literal option name.♻️ Proposed change
- with pytest.raises(PushRefused, match='collab.accept_push'): + with pytest.raises(PushRefused, match=re.escape('collab.accept_push')):Also applies to: 167-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_endpoint.py` at line 137, Update the pytest.raises match patterns in the affected tests to escape the dots in the literal collab.accept_push option name, using re.escape or an equivalent raw regex pattern, including the occurrences around the referenced test cases.Source: Linters/SAST tools
src/aiida/tools/collab/endpoint.py (1)
468-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
delta_idparameter to avoid shadowing the imported helper.
delta_idis imported fromaiida.tools.collab.protocolat Line 50 and is used at Lines 399 and 511. Insideresolve_deltathe name refers to the string parameter instead. The method does not call the helper today, so nothing breaks now; a later call inside this method would fail at runtime instead of at import.♻️ Proposed rename
- def resolve_delta(self, delta_id: str, requester: str = '') -> Path | None: + def resolve_delta(self, identifier: str, requester: str = '') -> Path | None: """Return the path of a negotiated delta, or ``None`` when nothing is on offer under that identifier. :param requester: the profile UUID of the peer, whose slot this download is activity of. """ with self._delta_lock: - cached = self._deltas.get(delta_id) + cached = self._deltas.get(identifier)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/collab/endpoint.py` around lines 468 - 484, Rename the resolve_delta parameter delta_id to a non-conflicting name, and update its use in self._deltas.get accordingly. Preserve the method’s public behavior and avoid changing the imported protocol helper or unrelated call sites.src/aiida/tools/collab/client.py (1)
264-282: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the streamed response on the 416 path.
_requestis called withstream=True, so the body is not read here. If the peer answers 416, the code returns or raises without consuming or closing the response. The underlying connection is then returned to the pool in an unread state, or held until garbage collection. Close the response on both 416 exits.♻️ Proposed change
if response.status_code == HTTPStatus.REQUESTED_RANGE_NOT_SATISFIABLE: # The only range the client asks for starts at the end of its partial file, so an unsatisfiable one # of the same size means the download is already complete. match = re.fullmatch(r'bytes \*/(\d+)', response.headers.get('Content-Range', '')) + response.close() if match is not None and int(match[1]) == offset: return 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/collab/client.py` around lines 264 - 282, Update the 416 handling in the delta download method around the streamed response from _request so response.close() is called before both the already-complete return and CollabRequestError raise paths. Keep the existing Content-Range validation and error behavior unchanged.src/aiida/tools/collab/sync.py (1)
1197-1219: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the bulk update in
_remap_hashes.When
uuidsisNone,apply_computer_mapremaps every calculation that ran on a mapped computer. This loop accumulates one row per calculation, and each row carries the full extras dict of that node. On a large profile the list grows without bound before a singlebulk_updateruns.
cmd_node.rehashinsrc/aiida/cmdline/commands/cmd_node.pysolves the same problem by flushing everyDEFAULT_BATCH_SIZErows while the query cursor is open. Apply the same pattern here.♻️ Proposed refactor
+ from aiida.common.utils import DEFAULT_BATCH_SIZE + rows = [] + written = 0 for label, node in query.iterall(): @@ extras = dict(node.base.extras.all) extras[NodeCaching._HASH_EXTRA_KEY] = remapped rows.append({'id': node.pk, 'extras': extras, 'mtime': node.mtime}) + if len(rows) >= DEFAULT_BATCH_SIZE: + backend.bulk_update(EntityTypes.NODE, rows) + written += len(rows) + rows = [] + if rows: backend.bulk_update(EntityTypes.NODE, rows) + written += len(rows) - return len(rows) + return writtenNote that the current
return len(rows)already returns the count of remapped calculations only because nothing flushes early; the refactor must keep that contract, whichwrittendoes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/tools/collab/sync.py` around lines 1197 - 1219, Update _remap_hashes to flush rows through backend.bulk_update(EntityTypes.NODE, rows) whenever the collection reaches DEFAULT_BATCH_SIZE, while the query cursor remains open, then clear the batch and track the total in a separate written counter. Flush any remaining rows after iteration and return written instead of len(rows), preserving the count of successfully remapped calculations.tests/tools/collab/test_state.py (1)
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuff B905: two
zipcalls omitstrict=. Both sites pair sequences whose lengths must match. Withoutstrict=, a future length mismatch truncates silently instead of failing.zip(strict=...)requires Python 3.10 or newer, so confirm the minimum version declared inpyproject.tomlbefore applying either fix.
tests/tools/collab/test_state.py#L87-L87: addstrict=Truetozip(times, directions), so that editing one list without the other fails the test instead of silently covering fewer events.src/aiida/tools/collab/sync.py#L580-L581: addstrict=Truetozip([group.uuid for group in missing], pks), so that a short return frombulk_insertfails instead of dropping group PKs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_state.py` at line 87, Confirm that pyproject.toml declares Python 3.10 or newer, then add strict=True to the zip call in tests/tools/collab/test_state.py at lines 87-87 within the loop pairing times and directions, and to the zip call in src/aiida/tools/collab/sync.py at lines 580-581 pairing missing group UUIDs with pks. Both sites must fail on length mismatches instead of truncating silently.Source: Linters/SAST tools
tests/tools/collab/test_sync.py (1)
667-667: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuff RUF059: seven tuple unpackings bind values that are never used. Each site destructures a tuple and discards one or more of the resulting names. Ruff fails on all of them. Prefix each unused name with an underscore, or unpack only the values the code reads.
tests/tools/collab/test_sync.py#L667-L667: rename the unusedfirstfromheavy_calculationto_first.tests/tools/collab/test_sync.py#L717-L717: rename the unusedfirstto_first.tests/tools/collab/test_sync.py#L767-L767: rename the unusedfirstto_first.tests/tools/collab/test_sync.py#L873-L873: rename the unusedfirstto_first.tests/tools/collab/test_sync.py#L931-L931: rename the unusedstate_onefrom thepeers('one')tuple to_state_one.tests/tools/collab/test_sync.py#L1020-L1020: rename the unusedstate_oneto_state_one.src/aiida/tools/collab/sync.py#L957-L958: unpack onlylink[0]andlink[1], and drop the unusedlink_typeandlabelnames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_sync.py` at line 667, Resolve Ruff RUF059 unused tuple bindings at all listed sites: in tests/tools/collab/test_sync.py lines 667, 717, 767, and 873 rename the unused heavy_calculation result first to _first, and at lines 931 and 1020 rename the unused peers('one') result state_one to _state_one; in src/aiida/tools/collab/sync.py lines 957-958 unpack only link[0] and link[1], dropping the unused link_type and label names.Source: Linters/SAST tools
tests/tools/collab/test_e2e_convergence.py (1)
23-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the docstring with what
membershipsreturns.
nodesandrefreshesare computed from events aftersince, butmembershipsis the total journal length. The docstring states all three are what the state "gained since an event index". Adjust the wording so a later reader does not treat the value as a delta.📝 Proposed wording
def transferred(member, since): - """Return the nodes, refreshes and memberships a member's state gained since an event index.""" + """Return the nodes and refreshes a member's state gained since an event index, and its total membership count."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_e2e_convergence.py` around lines 23 - 32, Update the docstring of transferred to clarify that nodes and refreshes describe additions since the event index, while memberships reports the member’s total membership count rather than a since-based delta.tests/tools/collab/test_e2e_membership.py (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the option constants instead of the literal strings.
OPTION_TOKENandOPTION_UUIDare already imported in other tests of this file. The literals'collab.token'and'collab.uuid'go stale silently if an option name changes.Also applies to: 183-183, 218-218, 275-275, 292-292, 354-354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_e2e_membership.py` at line 74, Replace the literal option names in the affected assertions with the imported OPTION_TOKEN and OPTION_UUID constants, including the usages around the membership and UUID checks. Keep the existing assertion behavior unchanged.tests/tools/collab/test_e2e_resume.py (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCentralize the partial-download filename
The CLI and
tests/tools/collab/test_e2e_resume.pyconstructpull-{peer_uuid}.aiidaindependently. Extract a shared helper or constant and use it in both locations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/collab/test_e2e_resume.py` around lines 33 - 35, Centralize construction of the partial-download filename used by the CLI and the test around the resume flow. Add a shared helper or constant for the pull filename pattern, then update the CLI and the test assertion in test_e2e_resume.py to use it instead of independently formatting pull-{peer_uuid}.aiida.src/aiida/cmdline/commands/cmd_status.py (1)
281-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
strict=Trueto thezipcall.Ruff reports B905 on this line.
infosalways has the same length aspeers, because line 277 maps overpeers.values()and line 273 leaves it empty only whenpeersis empty.strict=Truerecords that invariant and silences the lint.♻️ Proposed change
- for entry, info in zip(peers.values(), infos): + for entry, info in zip(peers.values(), infos, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/cmdline/commands/cmd_status.py` at line 281, Update the zip call in the status command’s peer-processing loop to pass strict=True, preserving the existing peers.values() and infos iteration while documenting their required equal lengths and resolving Ruff B905.Source: Linters/SAST tools
src/aiida/cmdline/commands/cmd_collab.py (1)
660-665: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSend the rotation signals concurrently.
The loop contacts each peer in sequence. Each unreachable peer costs
SIGNAL_TIMEOUTseconds, so the join code the user waits for is delayed by2.0 * <number of offline peers>.print_collab_statusinsrc/aiida/cmdline/commands/cmd_status.py(lines 275-277) solves the same problem with aThreadPoolExecutor. The signal is advisory, so failures stay warnings either way.♻️ Proposed refactor to probe peers concurrently
+ def signal(entry): + with CollabClient(entry['url'], retired, collab=collab, timeout=SIGNAL_TIMEOUT) as client: + try: + client.signal_retired(profile.uuid) + except CollabRequestError as exception: + return f'could not tell {entry["nickname"]} about the rotation: {exception}' + + return None + - for entry in active: - with CollabClient(entry['url'], retired, collab=collab, timeout=SIGNAL_TIMEOUT) as client: - try: - client.signal_retired(profile.uuid) - except CollabRequestError as exception: - echo.echo_warning(f'could not tell {entry["nickname"]} about the rotation: {exception}') + if active: + with ThreadPoolExecutor(max_workers=len(active)) as pool: + for warning in pool.map(signal, active): + if warning is not None: + echo.echo_warning(warning)Add
from concurrent.futures import ThreadPoolExecutorto the local imports at lines 635-639.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/cmdline/commands/cmd_collab.py` around lines 660 - 665, Update the rotation signaling flow around CollabClient and signal_retired to contact active peers concurrently using ThreadPoolExecutor, following the existing pattern in print_collab_status. Preserve per-peer warning behavior for CollabRequestError and ensure all submitted signals complete before the join flow proceeds.docs/source/howto/collaborate.rst (1)
105-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote that a hand-edited
collab.bindis not validated.
verdi collab initvalidates the address by binding it, so a wrong address fails immediately (line 57 states this).verdi config set collab.bindperforms no such check. If the user sets an address this machine does not hold, the endpoint fails at the next daemon start instead, and the only symptom is in the collab log. A short warning here would point the user at that log.📝 Proposed addition
If your address changes, correct ``collab.bind`` (or ``collab.port``), restart your daemon and sync once with any peer that is online — syncing outwards never depends on your own address. +Unlike ``verdi collab init``, ``verdi config set`` does not validate the address by binding it, so an address this machine does not hold only fails at the next daemon start; the reason is written to the collab log, which ``verdi bug-report`` collects. That announcement corrects the peer you contacted, and gossip carries the correction on to the others.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/howto/collaborate.rst` around lines 105 - 107, Add a brief warning to the documented hand-edited collab.bind guidance stating that verdi config set collab.bind does not validate the address, an invalid local address causes the daemon to fail on its next start, and users should check the collab log for the failure. Keep the existing verdi collab init validation note and surrounding guidance unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/cmdline/commands/cmd_bug_report.py`:
- Line 229: Update the collab request logging configuration near the filepaths
entry so BaseHTTPRequestHandler no longer records the full request line
containing query parameters. Override or adjust the relevant request-handler
logging behavior to emit only the HTTP method and URL path, preserving the
existing collab log and bug-report archive destinations.
In `@src/aiida/cmdline/commands/cmd_collab.py`:
- Around line 1313-1315: Guard every specified delta-file cleanup unlink in
src/aiida/cmdline/commands/cmd_collab.py: update lines 1313-1315, 1099-1100,
1349-1350, and 1376-1377 to use missing_ok=True for the delta and metadata paths
where applicable. Preserve the existing cleanup flow while ensuring absent
artifacts do not abort processing remaining peers.
In `@src/aiida/cmdline/commands/cmd_node.py`:
- Around line 532-538: Update the node batch construction and write flow around
NodeCaching._HASH_EXTRA_KEY and backend.bulk_update so concurrent changes are
not overwritten: modify only the hash extra, or perform an expected-mtime
compare-and-swap that reloads and retries on conflict. Do not submit the stale
full extras snapshot or mtime from rows.
In `@src/aiida/manage/configuration/config.py`:
- Line 905: Update the profile-removal flow around stop_daemon and delete_state
so deletion proceeds only after daemon termination is confirmed. When
stop_daemon times out or raises DaemonException, abort or otherwise coordinate
profile deletion with all collab writers instead of continuing to delete state,
work, and lock files.
In `@src/aiida/tools/collab/client.py`:
- Around line 400-407: Update the error-detail extraction in the
CollabRequestError handling block to catch TypeError alongside ValueError and
KeyError when accessing response.json()['detail'], preserving the fallback to
response.reason for non-object JSON responses.
In `@src/aiida/tools/collab/config.py`:
- Around line 247-255: Validate roster entries as dictionaries before any .get
access: guard the contact extraction from entries[0] and skip non-dict values at
the start of the loop in the roster merge logic. Preserve the existing handling
for incomplete dictionary entries and continue processing valid entries without
raising AttributeError.
In `@src/aiida/tools/collab/endpoint.py`:
- Around line 549-576: Ensure the push slot acquired for the handshake is
released when the withdrawn-consent check rejects the import: move the consent
validation into the existing try/finally in the import flow around import_delta,
while preserving the current PushRefused behavior and ensuring
_slots.release(f'push:{peer}') executes for both consent rejection and import
failures.
- Around line 173-185: Update the daemon control context around DaemonClient so
it captures client.is_daemon_running before issuing the stop command, and only
stops and restarts the daemon when it was running. Preserve the existing yield
behavior and avoid calling call_client when the captured state is false.
In `@src/aiida/tools/collab/protocol.py`:
- Around line 131-145: Update the decode method to validate that data['collab'],
data['url'], and data['token'] are nonempty strings before constructing
JoinCode. Raise ValueError for any invalid required field, while preserving the
existing policy validation and successful cls construction for valid payloads.
In `@src/aiida/tools/collab/server.py`:
- Around line 440-456: Update _put_upload to validate Content-Length before
indexing it, rejecting missing or invalid values with the same response and
close_connection behavior used by its existing refusal paths. Also validate the
declared body length against the byte range captured by match[2] and match[3],
including the total/last bounds, before opening _staging_path or writing any
bytes; reject mismatches early and preserve staging only for valid ranges.
In `@tests/tools/collab/conftest.py`:
- Around line 909-912: Add a targeted Ruff noqa for A002 to the overridden
log_message method’s format parameter, preserving the required
BaseHTTPRequestHandler signature and the existing no-op implementation.
In `@tests/tools/collab/test_e2e_archive_versions.py`:
- Around line 22-45: The test
test_a_thin_delta_at_an_older_format_is_migrated_and_imported incorrectly calls
faults.export_at_version with OLDER_VERSION, which is not supported as a
downgrade target. Remove or replace that call with a supported archive-version
setup that exercises migration to the older format without invoking the
forward-only downgrade path, while preserving the thin-delta boundary-link
scenario.
In `@tests/tools/collab/test_e2e_divergence.py`:
- Around line 127-137: Rename the unused third binding in the collab(3)
unpacking from c to an underscore placeholder, while preserving the existing a
and b bindings used by the test.
In `@tests/tools/collab/test_state.py`:
- Around line 55-66: Update the test setup around imported_uuids_since to create
explicitly distinct early and late instants using the existing timedelta-based
approach from the later test in this file, ensuring late is strictly after early
while preserving the current assertions.
In `@tests/tools/collab/test_sync.py`:
- Line 1327: Update the pytest.raises assertion around the collab.computer_map
error to escape the periods in its match pattern, ensuring the regular
expression matches the literal configuration option name and satisfies RUF043.
In `@tests/tools/collab/test_transport.py`:
- Around line 194-205: Wrap the yielded client/Transport fixture flow in a
finally block so server.shutdown(), server.server_close(), and thread.join()
always execute when the test body raises at yield. Preserve the existing setup
and yielded Transport values while ensuring cleanup occurs for both successful
and exceptional test completion.
---
Nitpick comments:
In `@docs/source/howto/collaborate.rst`:
- Around line 105-107: Add a brief warning to the documented hand-edited
collab.bind guidance stating that verdi config set collab.bind does not validate
the address, an invalid local address causes the daemon to fail on its next
start, and users should check the collab log for the failure. Keep the existing
verdi collab init validation note and surrounding guidance unchanged.
In `@src/aiida/cmdline/commands/cmd_collab.py`:
- Around line 660-665: Update the rotation signaling flow around CollabClient
and signal_retired to contact active peers concurrently using
ThreadPoolExecutor, following the existing pattern in print_collab_status.
Preserve per-peer warning behavior for CollabRequestError and ensure all
submitted signals complete before the join flow proceeds.
In `@src/aiida/cmdline/commands/cmd_status.py`:
- Line 281: Update the zip call in the status command’s peer-processing loop to
pass strict=True, preserving the existing peers.values() and infos iteration
while documenting their required equal lengths and resolving Ruff B905.
In `@src/aiida/manage/configuration/settings.py`:
- Around line 97-100: Update the new public callable docstrings to use Sphinx
directives and rely on annotations for types: add a :return: directive to
AiiDAConfigPathResolver.collab_dir in src/aiida/manage/configuration/settings.py
(97-100); convert is_enabled and the remaining new public callable docstrings in
src/aiida/tools/collab/config.py (47-51); convert JoinCode.encode,
JoinCode.decode, and the remaining new public callable docstrings in
src/aiida/tools/collab/protocol.py (107-121); and convert CollabState public
callable docstrings in src/aiida/tools/collab/state.py (142-152).
In `@src/aiida/tools/collab/client.py`:
- Around line 264-282: Update the 416 handling in the delta download method
around the streamed response from _request so response.close() is called before
both the already-complete return and CollabRequestError raise paths. Keep the
existing Content-Range validation and error behavior unchanged.
In `@src/aiida/tools/collab/endpoint.py`:
- Around line 468-484: Rename the resolve_delta parameter delta_id to a
non-conflicting name, and update its use in self._deltas.get accordingly.
Preserve the method’s public behavior and avoid changing the imported protocol
helper or unrelated call sites.
In `@src/aiida/tools/collab/sync.py`:
- Around line 1197-1219: Update _remap_hashes to flush rows through
backend.bulk_update(EntityTypes.NODE, rows) whenever the collection reaches
DEFAULT_BATCH_SIZE, while the query cursor remains open, then clear the batch
and track the total in a separate written counter. Flush any remaining rows
after iteration and return written instead of len(rows), preserving the count of
successfully remapped calculations.
In `@tests/cmdline/commands/test_status.py`:
- Around line 521-554: Replace the real loopback sockets and elapsed-time
assertion in test_status_collab_probe_timeout with a mocked CollabClient. Have
the mock record each supplied probe timeout and coordinate its info() calls with
synchronization primitives to verify probes execute concurrently; retain
assertions for six offline peers, the reachability summary, and the expected
timeout value.
In `@tests/tools/collab/test_e2e_convergence.py`:
- Around line 23-32: Update the docstring of transferred to clarify that nodes
and refreshes describe additions since the event index, while memberships
reports the member’s total membership count rather than a since-based delta.
In `@tests/tools/collab/test_e2e_failures.py`:
- Line 238: Update the failure assertion in the pull branch of the end-to-end
failure test to avoid matching the third-party archive-reader text; assert the
stable aiida-core wrapper message instead, or remove that branch while retaining
checks for the named peer, non-zero exit, and no landed provenance.
- Around line 322-327: Remove development-phase references from both test
docstrings: at tests/tools/collab/test_e2e_failures.py:322-327, remove the
“EXPECTED (phase 3):” prefix while preserving the behavior description; at
tests/tools/collab/test_e2e_failures.py:465-473, remove the “EXPECTED (phase
9):” prefix and the “See phase-15/deferred.md.” reference, while keeping the
explanation that expiry is pinned rather than the cap.
In `@tests/tools/collab/test_e2e_membership.py`:
- Line 74: Replace the literal option names in the affected assertions with the
imported OPTION_TOKEN and OPTION_UUID constants, including the usages around the
membership and UUID checks. Keep the existing assertion behavior unchanged.
In `@tests/tools/collab/test_e2e_resume.py`:
- Around line 33-35: Centralize construction of the partial-download filename
used by the CLI and the test around the resume flow. Add a shared helper or
constant for the pull filename pattern, then update the CLI and the test
assertion in test_e2e_resume.py to use it instead of independently formatting
pull-{peer_uuid}.aiida.
In `@tests/tools/collab/test_endpoint.py`:
- Line 137: Update the pytest.raises match patterns in the affected tests to
escape the dots in the literal collab.accept_push option name, using re.escape
or an equivalent raw regex pattern, including the occurrences around the
referenced test cases.
In `@tests/tools/collab/test_state.py`:
- Line 87: Confirm that pyproject.toml declares Python 3.10 or newer, then add
strict=True to the zip call in tests/tools/collab/test_state.py at lines 87-87
within the loop pairing times and directions, and to the zip call in
src/aiida/tools/collab/sync.py at lines 580-581 pairing missing group UUIDs with
pks. Both sites must fail on length mismatches instead of truncating silently.
In `@tests/tools/collab/test_sync.py`:
- Line 667: Resolve Ruff RUF059 unused tuple bindings at all listed sites: in
tests/tools/collab/test_sync.py lines 667, 717, 767, and 873 rename the unused
heavy_calculation result first to _first, and at lines 931 and 1020 rename the
unused peers('one') result state_one to _state_one; in
src/aiida/tools/collab/sync.py lines 957-958 unpack only link[0] and link[1],
dropping the unused link_type and label names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5707683c-0763-4f9e-869a-1f4b727abaa5
📒 Files selected for processing (46)
.gitignoreCLAUDE.mddocs/source/howto/collaborate.rstdocs/source/howto/index.rstdocs/source/reference/command_line.rstsrc/aiida/cmdline/commands/__init__.pysrc/aiida/cmdline/commands/cmd_archive.pysrc/aiida/cmdline/commands/cmd_bug_report.pysrc/aiida/cmdline/commands/cmd_collab.pysrc/aiida/cmdline/commands/cmd_daemon.pysrc/aiida/cmdline/commands/cmd_node.pysrc/aiida/cmdline/commands/cmd_status.pysrc/aiida/engine/daemon/client.pysrc/aiida/manage/configuration/config.pysrc/aiida/manage/configuration/settings.pysrc/aiida/orm/groups.pysrc/aiida/tools/collab/__init__.pysrc/aiida/tools/collab/client.pysrc/aiida/tools/collab/config.pysrc/aiida/tools/collab/endpoint.pysrc/aiida/tools/collab/protocol.pysrc/aiida/tools/collab/server.pysrc/aiida/tools/collab/state.pysrc/aiida/tools/collab/sync.pysrc/aiida/tools/graph/deletions.pytests/cmdline/commands/test_archive_import.pytests/cmdline/commands/test_bug_report.pytests/cmdline/commands/test_collab.pytests/cmdline/commands/test_node.pytests/cmdline/commands/test_status.pytests/manage/configuration/test_config.pytests/orm/test_groups.pytests/tools/collab/__init__.pytests/tools/collab/conftest.pytests/tools/collab/test_e2e_archive_versions.pytests/tools/collab/test_e2e_convergence.pytests/tools/collab/test_e2e_divergence.pytests/tools/collab/test_e2e_failures.pytests/tools/collab/test_e2e_membership.pytests/tools/collab/test_e2e_resume.pytests/tools/collab/test_endpoint.pytests/tools/collab/test_identity.pytests/tools/collab/test_state.pytests/tools/collab/test_sync.pytests/tools/collab/test_transport.pytests/tools/graph/test_deletions.py
| 'circus': filepaths['circus']['log'], | ||
| 'daemon': filepaths['daemon']['log'], | ||
| 'broker_service': filepaths['broker_service']['log'], | ||
| 'collab': filepaths['collab']['log'], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for token, authorization header, or join-code material reaching the collab log.
set -euo pipefail
echo '=== logging calls in the collab package ==='
rg -nP -C4 'LOGGER\.(debug|info|warning|error|exception|critical)' --type=py src/aiida/tools/collab
echo '=== token / auth header handling near logging ==='
rg -nP -C6 '\b(token|Authorization|bearer|Bearer|join_code|JoinCode)\b' --type=py src/aiida/tools/collab/server.py src/aiida/tools/collab/endpoint.py src/aiida/tools/collab/client.py
echo '=== does the HTTP server log request lines or headers? ==='
rg -nP -C6 'log_message|log_request|BaseHTTPRequestHandler|handle_one_request' --type=py src/aiida/tools/collabRepository: aiidateam/aiida-core
Length of output: 24540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== collab server request and logging implementation ==='
sed -n '190,292p' src/aiida/tools/collab/server.py
sed -n '530,558p' src/aiida/tools/collab/server.py
echo '=== all logger calls in collab sources ==='
rg -n -P 'LOGGER\.[A-Za-z_]+\s*\(' src/aiida/tools/collab --type=py
echo '=== bug-report log path construction and logger configuration ==='
sed -n '190,245p' src/aiida/cmdline/commands/cmd_bug_report.py
rg -n -P -C5 'collab|filepaths|log' src/aiida/cmdline/commands/cmd_bug_report.py src/aiida --glob '*.py' | head -n 300
echo '=== request-derived values used in exceptions/logging ==='
rg -n -P -C4 'raise |LOGGER\.|log_message|send_error|detail|str\(exception\)|self\.path|self\.headers|self\.rfile' src/aiida/tools/collab/server.py src/aiida/tools/collab/endpoint.pyRepository: aiidateam/aiida-core
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== collab secret and join-code references ==='
rg -n -P '\b(join[_ -]?code|token|authorization|bearer)\b' src/aiida/tools/collab src/aiida/cmdline --type=py --glob '!cmd_bug_report.py' | head -n 250
echo '=== collab logging calls with source expressions ==='
python3 - <<'PY'
import ast
from pathlib import Path
root = Path('src/aiida/tools/collab')
for path in sorted(root.glob('*.py')):
tree = ast.parse(path.read_text(), filename=str(path))
for node in ast.walk(tree):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute):
receiver = node.func.value
if isinstance(receiver, ast.Name) and receiver.id == 'LOGGER':
args = [ast.get_source_segment(path.read_text(), arg) for arg in node.args]
print(f'{path}:{node.lineno}: LOGGER.{node.func.attr}({", ".join(args)})')
PY
echo '=== logging configuration mentioning collab handlers or files ==='
rg -n -P -C5 'collab.*log|log.*collab|AIIDA_LOGGER|FileHandler|RotatingFileHandler' src/aiida --type=py | head -n 350
echo '=== request logging implementation references ==='
python3 - <<'PY'
import inspect
from http.server import BaseHTTPRequestHandler
for name in ('log_request', 'log_error', 'log_message', 'send_response', 'parse_request'):
method = getattr(BaseHTTPRequestHandler, name)
print(f'--- {name} ---')
print(inspect.getsource(method))
PYRepository: aiidateam/aiida-core
Length of output: 47434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== join-code transport and serialization ==='
sed -n '90,160p' src/aiida/tools/collab/protocol.py
sed -n '100,130p' src/aiida/tools/collab/client.py
sed -n '520,565p' src/aiida/cmdline/commands/cmd_collab.py
echo '=== endpoint report construction and logging ==='
sed -n '220,345p' src/aiida/tools/collab/endpoint.py
echo '=== logger argument data-flow candidates ==='
rg -n -P -C8 'report\(|reports\.append|LOGGER\.(report|debug|info|warning|error|exception)|JoinCode|join_code\(' src/aiida/tools/collab src/aiida/cmdline/commands/cmd_collab.py --type=pyRepository: aiidateam/aiida-core
Length of output: 50376
Stop logging the full request line. BaseHTTPRequestHandler.send_response() logs self.requestline, which includes query strings even for unauthenticated requests. A token or join code in the request URL is therefore copied to the collab log and bug-report archive. Log only the method and path, without the query string.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/cmdline/commands/cmd_bug_report.py` at line 229, Update the collab
request logging configuration near the filepaths entry so BaseHTTPRequestHandler
no longer records the full request line containing query parameters. Override or
adjust the relevant request-handler logging behavior to emit only the HTTP
method and URL path, preserving the existing collab log and bug-report archive
destinations.
Comment on lines
+1313
to
+1315
| if not uuids and not refresh and not members: | ||
| filepath.unlink() | ||
| filepath_meta.unlink() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded unlink() on delta files whose creation is not guaranteed. Both the push and the pull loop remove the per-peer delta artifact with a bare Path.unlink(). Neither call is reached through a path that proves the file exists when the transfer carried zero nodes. If the file is absent, FileNotFoundError escapes the surrounding handler — except CollabRequestError on the push side, except (ConfigurationError, IntegrityError) on the pull side — and aborts the whole loop, so every remaining peer is skipped. The etag unlink at line 1100 already uses missing_ok=True; apply the same treatment to the delta and metadata unlinks.
src/aiida/cmdline/commands/cmd_collab.py#L1313-L1315: passmissing_ok=Truetofilepath.unlink()andfilepath_meta.unlink()on the "nothing to push" path, which is reached exactly whenexport_deltaproduced zero UUIDs.src/aiida/cmdline/commands/cmd_collab.py#L1099-L1100: passmissing_ok=Truetofilepath.unlink()so it matches the etag unlink on the following line.
Apply the same guard to the other delta unlinks in the push loop at lines 1349-1350 and 1376-1377 if export_delta can return without writing a file.
📍 Affects 1 file
src/aiida/cmdline/commands/cmd_collab.py#L1313-L1315(this comment)src/aiida/cmdline/commands/cmd_collab.py#L1099-L1100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/cmdline/commands/cmd_collab.py` around lines 1313 - 1315, Guard
every specified delta-file cleanup unlink in
src/aiida/cmdline/commands/cmd_collab.py: update lines 1313-1315, 1099-1100,
1349-1350, and 1376-1377 to use missing_ok=True for the delta and metadata paths
where applicable. Preserve the existing cleanup flow while ensuring absent
artifacts do not abort processing remaining peers.
Comment on lines
+532
to
+538
| extras = dict(node.base.extras.all) | ||
| extras[NodeCaching._HASH_EXTRA_KEY] = node.base.caching.compute_hash() | ||
| rows.append({'id': node.pk, 'extras': extras, 'mtime': node.mtime}) | ||
|
|
||
| if len(rows) >= DEFAULT_BATCH_SIZE: | ||
| backend.bulk_update(EntityTypes.NODE, rows) | ||
| rows = [] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent a stale batch from overwriting concurrent node changes.
Line 532 snapshots all extras and Line 534 snapshots mtime before the batch is written. If another session updates the node before Line 537, this bulk update overwrites that newer extras value and restores the older mtime.
Use a concurrency-safe update that changes only the hash extra, or use an expected-mtime compare-and-swap with a reload and retry on conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/cmdline/commands/cmd_node.py` around lines 532 - 538, Update the
node batch construction and write flow around NodeCaching._HASH_EXTRA_KEY and
backend.bulk_update so concurrent changes are not overwritten: modify only the
hash extra, or perform an expected-mtime compare-and-swap that reloads and
retries on conflict. Do not submit the stale full extras snapshot or mtime from
rows.
| else: | ||
| LOGGER.report(f'Data storage not deleted, configuration is: {profile.storage_config}') | ||
|
|
||
| delete_state(profile) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not delete collab state while the daemon can still write it.
If stop_daemon times out or raises DaemonException, the earlier branches only log the failure and continue. Line 905 then deletes the state file, work directory, and lock files while the daemon can still mutate them. A later state write recreates collab artifacts after profile deletion. Removing a held lock file can also permit concurrent state mutations.
Abort deletion until daemon exit is confirmed, or coordinate deletion with all collab writers through profile removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/manage/configuration/config.py` at line 905, Update the
profile-removal flow around stop_daemon and delete_state so deletion proceeds
only after daemon termination is confirmed. When stop_daemon times out or raises
DaemonException, abort or otherwise coordinate profile deletion with all collab
writers instead of continuing to delete state, work, and lock files.
Comment on lines
+400
to
+407
| if response.status_code >= HTTPStatus.BAD_REQUEST and response.status_code not in allowed: | ||
| try: | ||
| detail = response.json()['detail'] | ||
| except (ValueError, KeyError): | ||
| detail = response.reason | ||
|
|
||
| msg = f'the peer at {self._base_url} responded {response.status_code} to {method} {route}: {detail}' | ||
| raise CollabRequestError(msg, status=response.status_code) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Catch TypeError when reading the error detail.
response.json() can return a list, a string, or a number for a service that is not a collab endpoint. Indexing that with 'detail' raises TypeError, which this handler does not catch, so the failure escapes as TypeError instead of CollabRequestError. _answer at Line 385 already guards the same shape.
🐛 Proposed fix
try:
detail = response.json()['detail']
- except (ValueError, KeyError):
+ except (KeyError, TypeError, ValueError):
detail = response.reason📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if response.status_code >= HTTPStatus.BAD_REQUEST and response.status_code not in allowed: | |
| try: | |
| detail = response.json()['detail'] | |
| except (ValueError, KeyError): | |
| detail = response.reason | |
| msg = f'the peer at {self._base_url} responded {response.status_code} to {method} {route}: {detail}' | |
| raise CollabRequestError(msg, status=response.status_code) | |
| if response.status_code >= HTTPStatus.BAD_REQUEST and response.status_code not in allowed: | |
| try: | |
| detail = response.json()['detail'] | |
| except (KeyError, TypeError, ValueError): | |
| detail = response.reason | |
| msg = f'the peer at {self._base_url} responded {response.status_code} to {method} {route}: {detail}' | |
| raise CollabRequestError(msg, status=response.status_code) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/tools/collab/client.py` around lines 400 - 407, Update the
error-detail extraction in the CollabRequestError handling block to catch
TypeError alongside ValueError and KeyError when accessing
response.json()['detail'], preserving the fallback to response.reason for
non-object JSON responses.
Comment on lines
+22
to
+45
| # The archive revision before the current head, which is what an older aiida-core writes. | ||
| OLDER_VERSION = 'main_0000' | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('direction', DIRECTIONS) | ||
| def test_a_thin_delta_at_an_older_format_is_migrated_and_imported(collab, faults, direction): | ||
| """Test that a thin delta written at an older archive format is migrated forward, boundary links and all. | ||
|
|
||
| The case ``phase-11/deferred.md`` recorded as untested: the migration was only ever exercised against a | ||
| full-closure archive, and the boundary links a collab depends on live in a metadata key of its own that no | ||
| migration knows about. | ||
| """ | ||
| a, b, _ = collab(3) | ||
| first = a.seal_calculation() | ||
|
|
||
| move(a, b, direction) | ||
|
|
||
| # A second generation whose input node Bob already holds, so the delta is thin and its link crosses the | ||
| # boundary rather than travelling as a row. | ||
| second = a.seal_calculation(inputs=first) | ||
|
|
||
| faults.export_at_version(OLDER_VERSION) | ||
|
|
||
| move(a, b, direction) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the archive migrator for backward-migration support and the `main_0000` revision.
fd -t f 'migrations' src/aiida/storage --exec echo {} \;
rg -n -C5 'def migrate' src/aiida/tools/archive/abstract.py
rg -rn -C5 'def migrate\b' src/aiida/storage/sqlite_zip/ 2>/dev/null | head -80
rg -n 'main_0000' src/aiida --glob '*.py' | head -40Repository: aiidateam/aiida-core
Length of output: 4320
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- abstract migration contract ---'
sed -n '220,285p' src/aiida/storage/migrations.py
printf '%s\n' '--- sqlite_zip migrator ---'
sed -n '1,180p' src/aiida/storage/sqlite_zip/migrator.py
printf '%s\n' '--- archive format and test fixture ---'
rg -n -C8 'def export_at_version|get_format\(\)\.migrate|class .*Format|def migrate' tests/tools/collab/conftest.py src/aiida/tools/archive src/aiida/storage/sqlite_zip
printf '%s\n' '--- migration graph and downgrade implementations ---'
sed -n '1,240p' src/aiida/storage/sqlite_zip/migrations/versions/main_0000_initial.py
sed -n '1,120p' src/aiida/storage/sqlite_zip/migrations/versions/main_0000a_replace_nulls.py
sed -n '1,110p' src/aiida/storage/sqlite_zip/migrations/versions/main_0000b_non_nullable.pyRepository: aiidateam/aiida-core
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- version validation ---'
rg -n -C12 'def validate_archive_versions|get_current_archive_version' src/aiida/storage/sqlite_zip/backend.py
printf '%s\n' '--- Alembic migration execution ---'
sed -n '150,360p' src/aiida/storage/sqlite_zip/migrator.py
printf '%s\n' '--- archive-format delegation ---'
sed -n '68,95p' src/aiida/tools/archive/implementations/sqlite_zip/main.py
printf '%s\n' '--- migration heads and downgrade markers ---'
rg -n '^(revision|down_revision|def downgrade)|NotImplementedError.*Downgrade' src/aiida/storage/sqlite_zip/migrations/versions --glob '*.py'Repository: aiidateam/aiida-core
Length of output: 15464
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import ast
from pathlib import Path
versions_dir = Path('src/aiida/storage/sqlite_zip/migrations/versions')
revisions = {}
for path in sorted(versions_dir.glob('*.py')):
tree = ast.parse(path.read_text())
values = {}
for node in tree.body:
if isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name):
if node.targets[0].id in {'revision', 'down_revision'}:
try:
values[node.targets[0].id] = ast.literal_eval(node.value)
except Exception:
pass
if values.get('revision'):
revisions[values['revision']] = (values.get('down_revision'), path)
print('migration chain:')
for revision, (parent, path) in revisions.items():
if revision.startswith('main_'):
print(f'{revision} <- {parent} ({path})')
target = 'main_0000'
head_candidates = [
revision for revision, (parent, _) in revisions.items()
if revision.startswith('main_') and revision not in {p for p, _ in revisions.values() if isinstance(p, str)}
]
print(f'head candidates: {head_candidates}')
print(f'target downgrade implementations:')
for revision, (_, path) in revisions.items():
if revision.startswith('main_') and revision != target:
tree = ast.parse(path.read_text())
downgrades = [
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == 'downgrade'
]
body = downgrades[0].body if downgrades else []
raises_not_implemented = any(
isinstance(node, ast.Raise)
and isinstance(node.exc, ast.Call)
and isinstance(node.exc.func, ast.Name)
and node.exc.func.id == 'NotImplementedError'
for node in ast.walk(ast.Module(body=body, type_ignores=[]))
)
print(f'{revision}: raises NotImplementedError = {raises_not_implemented}')
PYRepository: aiidateam/aiida-core
Length of output: 761
Do not target main_0000 with faults.export_at_version.
The archive migration chain is forward-only: main_0000 -> main_0000a -> main_0000b -> main_0001, and later revisions raise NotImplementedError from downgrade(). This call cannot create an older archive and the test fails before exercising boundary links.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools/collab/test_e2e_archive_versions.py` around lines 22 - 45, The
test test_a_thin_delta_at_an_older_format_is_migrated_and_imported incorrectly
calls faults.export_at_version with OLDER_VERSION, which is not supported as a
downgrade target. Remove or replace that call with a supported archive-version
setup that exercises migration to the older format without invoking the
forward-only downgrade path, while preserving the thin-delta boundary-link
scenario.
Comment on lines
+127
to
+137
| a, b, c = collab(3) | ||
| created = b.seal_calculation() | ||
|
|
||
| # Carol's copy comes through Alice, so Carol's first contact with Bob is unbounded and offers everything she | ||
| # holds — which is the only way work Bob produced himself can be handed back to him at all. | ||
| b.run('push', ['alice', '--force']) | ||
| a.run('push', ['carol', '--force']) | ||
|
|
||
| faults.delete_during_negotiation(b, created) | ||
|
|
||
| b.run('pull', ['carol', '--force']) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the unused unpacked member to satisfy Ruff RUF059.
The test refers to Carol by nickname only, so the c binding is unused.
🔧 Proposed fix
- a, b, c = collab(3)
+ a, b, _ = collab(3)
created = b.seal_calculation()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| a, b, c = collab(3) | |
| created = b.seal_calculation() | |
| # Carol's copy comes through Alice, so Carol's first contact with Bob is unbounded and offers everything she | |
| # holds — which is the only way work Bob produced himself can be handed back to him at all. | |
| b.run('push', ['alice', '--force']) | |
| a.run('push', ['carol', '--force']) | |
| faults.delete_during_negotiation(b, created) | |
| b.run('pull', ['carol', '--force']) | |
| a, b, _ = collab(3) | |
| created = b.seal_calculation() | |
| # Carol's copy comes through Alice, so Carol's first contact with Bob is unbounded and offers everything she | |
| # holds — which is the only way work Bob produced himself can be handed back to him at all. | |
| b.run('push', ['alice', '--force']) | |
| a.run('push', ['carol', '--force']) | |
| faults.delete_during_negotiation(b, created) | |
| b.run('pull', ['carol', '--force']) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 127-127: Unpacked variable c is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools/collab/test_e2e_divergence.py` around lines 127 - 137, Rename the
unused third binding in the collab(3) unpacking from c to an underscore
placeholder, while preserving the existing a and b bindings used by the test.
Source: Linters/SAST tools
Comment on lines
+55
to
+66
| early, late = timezone.now(), timezone.now() | ||
| state = CollabState( | ||
| filepath=tmp_path / 'state.json', | ||
| events=[ | ||
| CollabEvent(time=early, direction='pull', peer=PEER, uuids=['uuid-early'], size=1), | ||
| CollabEvent(time=late, direction='push', peer=PEER, uuids=['uuid-pushed'], size=1), | ||
| CollabEvent(time=late, direction='pull', peer='http://other:9137', uuids=['uuid-late'], size=1), | ||
| ], | ||
| ) | ||
|
|
||
| assert state.imported_uuids_since(None) == {'uuid-early', 'uuid-late'} | ||
| assert state.imported_uuids_since(late) == {'uuid-late'} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the two instants explicitly distinct at Line 55.
early and late come from two consecutive timezone.now() calls. If the clock resolution returns the same value twice, early == late. The filter in _uuids_since is event.time >= instant, so the assertion at Line 66 would then also see uuid-early and fail.
Line 83 of this same file already builds distinct instants with timedelta. Use the same approach here.
💚 Proposed fix
+ from datetime import timedelta
+
- early, late = timezone.now(), timezone.now()
+ early = timezone.now()
+ late = early + timedelta(seconds=1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| early, late = timezone.now(), timezone.now() | |
| state = CollabState( | |
| filepath=tmp_path / 'state.json', | |
| events=[ | |
| CollabEvent(time=early, direction='pull', peer=PEER, uuids=['uuid-early'], size=1), | |
| CollabEvent(time=late, direction='push', peer=PEER, uuids=['uuid-pushed'], size=1), | |
| CollabEvent(time=late, direction='pull', peer='http://other:9137', uuids=['uuid-late'], size=1), | |
| ], | |
| ) | |
| assert state.imported_uuids_since(None) == {'uuid-early', 'uuid-late'} | |
| assert state.imported_uuids_since(late) == {'uuid-late'} | |
| from datetime import timedelta | |
| early = timezone.now() | |
| late = early + timedelta(seconds=1) | |
| state = CollabState( | |
| filepath=tmp_path / 'state.json', | |
| events=[ | |
| CollabEvent(time=early, direction='pull', peer=PEER, uuids=['uuid-early'], size=1), | |
| CollabEvent(time=late, direction='push', peer=PEER, uuids=['uuid-pushed'], size=1), | |
| CollabEvent(time=late, direction='pull', peer='http://other:9137', uuids=['uuid-late'], size=1), | |
| ], | |
| ) | |
| assert state.imported_uuids_since(None) == {'uuid-early', 'uuid-late'} | |
| assert state.imported_uuids_since(late) == {'uuid-late'} |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 60-60: Do not make http calls without encryption
Context: 'http://other:9137'
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools/collab/test_state.py` around lines 55 - 66, Update the test setup
around imported_uuids_since to create explicitly distinct early and late
instants using the existing timedelta-based approach from the later test in this
file, ensuring late is strictly after early while preserving the current
assertions.
| filepath = tmp_path / 'delta.aiida' | ||
| export = export_full(filepath, state=state_one, backend=backend_one, cursor=None, claim=held) | ||
|
|
||
| with pytest.raises(ConfigurationError, match='collab.computer_map'): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the match pattern at Line 1327.
Ruff reports RUF043. match= takes a regular expression, and the . characters match any character. Escape them so the assertion tests the literal option name.
♻️ Proposed fix
- with pytest.raises(ConfigurationError, match='collab.computer_map'):
+ with pytest.raises(ConfigurationError, match=r'collab\.computer_map'):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with pytest.raises(ConfigurationError, match='collab.computer_map'): | |
| with pytest.raises(ConfigurationError, match=r'collab\.computer_map'): |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 1327-1327: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools/collab/test_sync.py` at line 1327, Update the pytest.raises
assertion around the collab.computer_map error to escape the periods in its
match pattern, ensuring the regular expression matches the literal configuration
option name and satisfies RUF043.
Source: Linters/SAST tools
Comment on lines
+194
to
+205
| server = build_server('127.0.0.1', 0, stub, staging_dir) | ||
| thread = threading.Thread(target=server.serve_forever, daemon=True) | ||
| thread.start() | ||
|
|
||
| url = f'http://127.0.0.1:{server.server_address[1]}' | ||
|
|
||
| with CollabClient(url, TOKEN, collab=COLLAB, peer=PEER, timeout=10) as client: | ||
| yield Transport(client=client, stub=stub, staging_dir=staging_dir, url=url) | ||
|
|
||
| server.shutdown() | ||
| server.server_close() | ||
| thread.join() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Shut the server down in a finally block.
If a test body raises, the exception is thrown into the generator at the yield. Lines 203-205 never run, so serve_forever keeps running and the port stays bound for the rest of the session. Each failing test leaks one listener thread.
♻️ Proposed change
- with CollabClient(url, TOKEN, collab=COLLAB, peer=PEER, timeout=10) as client:
- yield Transport(client=client, stub=stub, staging_dir=staging_dir, url=url)
-
- server.shutdown()
- server.server_close()
- thread.join()
+ try:
+ with CollabClient(url, TOKEN, collab=COLLAB, peer=PEER, timeout=10) as client:
+ yield Transport(client=client, stub=stub, staging_dir=staging_dir, url=url)
+ finally:
+ server.shutdown()
+ server.server_close()
+ thread.join()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| server = build_server('127.0.0.1', 0, stub, staging_dir) | |
| thread = threading.Thread(target=server.serve_forever, daemon=True) | |
| thread.start() | |
| url = f'http://127.0.0.1:{server.server_address[1]}' | |
| with CollabClient(url, TOKEN, collab=COLLAB, peer=PEER, timeout=10) as client: | |
| yield Transport(client=client, stub=stub, staging_dir=staging_dir, url=url) | |
| server.shutdown() | |
| server.server_close() | |
| thread.join() | |
| server = build_server('127.0.0.1', 0, stub, staging_dir) | |
| thread = threading.Thread(target=server.serve_forever, daemon=True) | |
| thread.start() | |
| url = f'http://127.0.0.1:{server.server_address[1]}' | |
| try: | |
| with CollabClient(url, TOKEN, collab=COLLAB, peer=PEER, timeout=10) as client: | |
| yield Transport(client=client, stub=stub, staging_dir=staging_dir, url=url) | |
| finally: | |
| server.shutdown() | |
| server.server_close() | |
| thread.join() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools/collab/test_transport.py` around lines 194 - 205, Wrap the
yielded client/Transport fixture flow in a finally block so server.shutdown(),
server.server_close(), and thread.join() always execute when the test body
raises at yield. Preserve the existing setup and yielded Transport values while
ensuring cleanup occurs for both successful and exceptional test completion.
`_dispatch` sets `close_connection` on an unauthenticated request but never sent `Connection: close` with the answer, unlike the 409 beneath it. The body of such a request is never read, so the socket goes either way; a client that pools connections — `requests`, which `CollabClient` uses — is simply not told, and whether its next request on that session gets a clean answer or a `RemoteDisconnected` depends on whether the FIN arrived before the pool checked the socket. That answer is the one that matters most: a member whose token this endpoint retired reaches exactly the 401, and its detail is the only place a rotation is ever explained. Losing it to a connection error leaves that member with nothing pointing at the rekey.
`refresh_offer` answered a null cursor with nothing, reasoning that such a peer holds nothing of this profile and so has no extras to refresh. That is false for every node the peer gave this profile in the first place: it holds those already, and their extras ride in no delta. The first exchange therefore carried no refresh and still advanced the cursor past the edit, after which every later offer — bounded by that cursor — excluded it too. The most natural use of `extras_mode: sync` runs straight into it: A produces and shares, B annotates what it pulled, and A never receives the annotation, with nothing in the output, the log or `verdi status` to say so. A null cursor is now answered with the mtime of every node. Over-stating was always this offer's safety model — the receiver holds the authoritative comparison and asks only for the snapshots it turns out to need — and the cost is smaller than it looks, since a null-cursor negotiation already ships a manifest of the whole shareable graph. The push direction needed a second half. Its cursor is written by an import and by nothing else, so an empty push, short-circuited before any import, left the pusher presenting a null cursor forever and gave an extras-only change no route at all. An empty push against a peer without a cursor now rides through the upload and the import once, as the zero-node pull already did; against a peer that has one it is still short-circuited. Nothing travelling means nothing to confirm, so the prompt is skipped there, as the pull's already is.
Every handshake and every negotiation carried the whole tombstone set, so a profile that deleted a large campaign put that set on the wire at each contact, forever, and it keyed the delta cache besides. The bound was O(history) for a defence about one sync. It is also the wrong place. Since the thin deltas of phase 8 the sender no longer decides what travels: it serves a manifest and the requester asks for the subset it lacks. A tombstoned node is missing locally, so it survives that diff — the diff is where the refusal belongs, bounded by the delta. The claim is now the imported nodes alone, and the receiver answers `refuse = missing & tombstones`, `want = missing - refuse`. Only what is *absent* may be refused: a node held and tombstoned, which is what a restoration leaves behind, must keep being linked to. The sender then has to close the cut, because subtracting naively breaks two ways. A refused node the wanted provenance requires would become a boundary link to something that exists nowhere — and the receiver's own import re-filter closes over it regardless — so `required_refused` puts it back, under the re-filter's own traversal rules. A refused node that is *not* required must not become a boundary link either, or `_check_boundary_resolvable` would abort the whole import; `export_delta` drops those links instead, and the output lands creator-less, which is what the same graph already produced. Both queries run only when something was refused. `delta_id` gains the refusal beside the want, since the dropped-link rule makes the archive depend on it even where the effective want is identical. `--include-deleted` refuses nothing — that is what "do not subtract" means — and keeps subtracting the tombstones from its claim. `import_delta` is untouched: its filter stays as the backstop against a diverged sender and against a deletion made while a delta was in flight. The state file still grows and is still never pruned. What a deletion costs on the wire is now bounded by the sync at hand.
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
1 participant
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.
verdi collab: share one provenance graph between profilesImplements #7481.
A collab is a set of AiiDA profiles, on different machines inside one trusted private network (WireGuard, Tailscale, an institute VLAN), that share one logical provenance graph. There is no server and no coordinator: any pair of members syncs directly, and pairwise contacts alone converge the whole group in any topology — provenance produced on A reaches C through B without C ever contacting A.
Everything is opt-in and off by default: without
collab.enabledno endpoint is started, no hook fires and no configuration migration is needed — the collab options are ordinary profile options with defaults. No new dependencies.The model
ProcessNode, and anything whose provenance reaches one, stays home until it seals.syncpolicy.Architecture
Two properties carry the design.
server.pytouches no ORM, no storage backend and no profile — every behaviour is a callable injected by the endpoint, which is what lets the transport be tested against stubs and the sync core with no socket. And the CLI and the endpoint run the samesync.py: a pull and a received push land through oneimport_delta; a push cut and a served pull go through onecompute_delta+export_delta. There is no second implementation on the "server side".State lives in three places: the
collab.*profile options inconfig.json(identity, token, roster, bind, policy); a state sidecar per profile UUID (cursors,tombstones, the event log, journalled boundary links and memberships) kept out ofconfig.jsonbecause it grows with every sync; and a workdir holding resumable uploads, downloads and exported deltas.The wire
Plain HTTP/1.1 with the token as a bearer header — the private overlay is the encryption, and the server refuses to bind the wildcard address for exactly that reason. The unit of transfer is a thin delta archive: an
sqlite_ziparchive holding exactly the nodes the receiver asked for, deliberately not provenance-closed, carrying the links that cross its boundary as UUID quadruples in the archive metadata, re-attached on import.verdi archive importrefuses such an archive, since it would silently lose them.A pull is: handshake (versions, policy, identities) → negotiate a manifest for my cursor and claim → diff it against what I hold → confirm → export, download (resumable,
Range/ETag) → import under a lock, advance the cursor. A push is the mirror, with the receiver answering busy at the handshake — before anything is exported or uploaded — and the upload staged content-addressed, so a failed import retries the import alone and transfers zero bytes.compute_deltaseeds from sealed processes at or after the cursor ∪ nodes this profile imported since it, minus what the requester claims to hold, minus anything whose provenance reaches an unsealed process, then takes the provenance closure. That union is what makes relaying converge: imported nodes keep the original profile's timestamps, so an mtime bound alone would silently drop everything B relayed from C.Policy, identity, safety
Policy (
extras_mode,groups_mode) is chosen at creation, travels in the join code, and is immutable. Underextras sync, extras of shared nodes replicate as whole-dict snapshots, newest node mtime wins, with_-prefixed keys exempt from that replication in both directions (a node's first export still carries its own extras, minus the caching ones, which never travel under either policy). Undergroups grow, curating a node into a group propagates that membership — additions only. Both gates sit on the import side and read the local value, so whatever a peer declares or serves, this profile decides what enters it.Three identities with three lifetimes: the collab UUID (permanent, refuses to splice two collabs), the profile UUID (permanent identity of a member; cursors, roster and sessions are keyed by it, and it survives any change of address), and the token (the replaceable key). Addresses spread by gossip on every sync, where only the owner may raise its own stamp — that makes "whose information is newer" a purely local decision, with no consensus and no vector clocks.
rotate/rekeymake every peer dormant until it reappears under the current token; dormant peers keep their cursors, so rejoining transfers nothing twice.Deletions never propagate in either direction;
delete_nodesrecords tombstones that keep a node from being re-offered. Imports are serialized by a file lock (which is also what lets a handshake answer busy), and concurrent serving is capped by expiring slots. On SQLite a received push pauses the workers around its import, whileverdi collab pullrefuses to import against a running daemon unless--pause-my-daemonis passed. Credentials never travel: computers arrive as entities,AuthInfodoes not. Opt-incollab.computer_mapremaps an imported calculation's hash onto a local computer so a pulled calculation can be a cache hit.Tests and docs
~6k lines of implementation against ~10k of tests.
tests/tools/collab/test_e2e_*.pyruns three real profiles with real endpoints on loopback, driving the real CLI, and every synced feature is covered by a three-party propagation test (produced on A, delivered to C only through pairwise contacts with B). Unit files beside them cover one layer at a time: state, identity, transport, endpoint, sync core. Nothing needs a second machine.docs/source/howto/collaborate.rstdocuments the trust model, setup, both policies and the known restrictions — the trust boundary in particular: any token holder can pull everything the negotiation entitles them to, membership is collab-level, and exclusion completes only when the last member rekeys.