Refuse a launch whose derived id collides with a different triple - #524
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR prevents launches from silently sharing a clone directory and DevPod workspace when distinct repository/ref triples derive the same lossy workspace ID, using a read-only metadata lookup before launch, an actionable refusal API and renderer, and a corresponding migration fix that blocks duplicate destinations. Sequence diagram for refusing a colliding workspace launchsequenceDiagram
participant User
participant Launch
participant MetadataStorage
participant DevPod
User->>Launch: run(owner/repo@branch)
Launch->>MetadataStorage: recorded()
MetadataStorage-->>Launch: existing worktree records
alt different triple holds derived workspace ID
Launch-->>User: LaunchRefusal::IdCollision
else no collision or unreadable store
Launch->>DevPod: attach or launch workspace
DevPod-->>Launch: session result
Launch-->>User: launch result
end
Flow diagram for derived workspace ID collision handlingflowchart TD
Start[Derive workspace ID] --> Read[Read recorded worktrees]
Read --> Available{Different triple holds ID?}
Available -->|Yes| Refuse[Return IdCollision refusal]
Refuse --> Explain[Render both triples, shared ID, and rename guidance]
Available -->|No or records unreadable| Proceed[Continue launch]
Proceed --> Attach[Attach or create workspace]
Flow diagram for collision-safe metadata migrationflowchart TD
Begin[Start migration] --> Seed[Seed claimed paths from existing records]
Seed --> Record[Process next worktree record]
Record --> Destination[Derive destination path]
Destination --> Claimed{Destination already claimed?}
Claimed -->|Yes| Block[Report blocked; keep record on its own clone]
Claimed -->|No| Take[Claim destination and update record]
Block --> More{More records?}
Take --> More
More -->|Yes| Record
More -->|No| Done[Complete migration]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
fd46f65 to
4a27f9a
Compare
|
Rebased onto af9f5a8 (head The CHANGELOG. Main cut 0.25.0 mid-branch, so the conflict resolves plausibly and wrongly by keeping both sides in order, which files this change inside a release that does not contain it. The entry is in Why CI never ran on the first push. The PR was The public-API snapshot was re-derived on top of main's own snapshot change (456f917), not hand-merged: the diff against main is exactly the ten rows this branch adds and nothing else. Local on the rebased head: Also smoke-tested the release binary against a scratch
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
a52d302 to
d442aca
Compare
A workspace id is `<repo-slug>-<ref-slug>-<suffix>`, and only the four character suffix makes it injective: the owner is not in the readable half at all, `slug()` collapses `feature/auth` and `feature-auth` to one string, and a long ref loses its tail to the 47 character cap. Two triples whose ids land on the same string share both things the id names, because devpod workspace names are global rather than scoped by repository: one clone directory and one container. Nothing detected that. The second launch attached to the first one's container and said nothing, so the user opened somebody else's checkout from a command with no reason to look wrong, and a later `dl <ws> rm` on either one deleted a clone the other still claimed. The suffix width was the whole of the defence, which the two doc comments in workspace_id.rs said in as many words. `place_triple` now scans the records for a record holding the derived id under a different triple, before devpod is asked anything and before the cold path can build a directory, and refuses with both triples and the id. Three fail-open rules, each with a test: a record whose branch is not a legal ref is skipped rather than failed on (the old derivation coerced unsafe refs, so a stored branch is not necessarily one), a store that cannot be read means "no collision" rather than an error, and a launch matching its own record attaches exactly as before. The warm path keeps what devlaunch#145 bought it. `ColdMachinery` grows `recorded`, which reads `metadata.json` through the new `MetadataStorage::look`: no lock, no migration, no `config.toml`, no clone manager, and no write of any kind. `look` is a second constructor rather than a flag on `open` because `open`'s three writes -- creating the cache directory, quarantining an unusable file, backing up one that will not round-trip -- all exist to protect bytes from the caller's next save, and a reader has no next save. Also closes the migration half named in the ticket's own comment. The schema 2 to 3 migration built `claimed` from the pre-run layout alone, so two records deriving one destination both ended up owning it: the shared path was new, no record had pointed at it before the run, and the second record found it existing and repointed itself. `claimed` now grows as destinations are taken, so the second lands in `blocked` like any other conflict. The collision the tests are written against is a real one, found by searching the shape the module doc records seeing in the wild. Closes #438
The collision guard read "a different triple" as "a different pair of strings", and the derivation does not. `WorkspaceId::suffix` lowercases owner and repo before hashing, deliberately: GitHub's owners and repos are case-insensitive, so `NVIDIA/cuda-samples` and `nvidia/cuda-samples` are one repository and derive one id, and the alternative is one repo cloned twice into two containers. So the second spelling of a repository derived the first spelling's id and was then refused for holding it. Nothing normalises earlier -- `worktree_key` interpolates the raw strings -- so before the guard existed the record lookup missed on case and both spellings converged on the one workspace, which is the designed behaviour. The refusal broke it, and broke it with a message naming no way out: it says to rename one of the two branches, and both branches are `main`. The reader is locked out of a workspace they already have running until they guess the case they first typed. `identity_of` is now the one place that rule lives. `suffix` hashes it and the guard compares it, so the two cannot drift into disagreeing about which spellings are one workspace -- which is exactly what went wrong. Owner and repo fold; the ref does not, because git refs really are case-sensitive and `Main` and `main` can both exist. The `holds_id` arm that re-derives from a record's triple already went through `WorkspaceId::new` and so was already right about the id; it was the comparison beside it that disagreed. Tests both ways: the other spelling of a repository is not refused, and a ref differing only in case still is. The second has to stage its collision, since two refs differing in case hash apart by design. Also renames `a_warm_triple_launch_does_no_metadata_io_at_all`. Every assertion in it is about writes and they all still hold, but the name claimed no I/O when the guard does read the file, and a name that over-claims invites someone to "fix" the read. It is now `a_warm_triple_launch_writes_nothing_to_the_cache`, with a line saying the read is deliberate.
d442aca to
65ea76f
Compare
Taken from CI's regenerate-and-diff rather than guessed at: one row in public-api.api.txt and three in public-api.rest.txt, at the positions the generator puts them. Worth saying out loud, because the api file is the promised tier: `ColdMachinery` is re-exported at `devlaunch_core::api`, so adding a method to it widens the promised contract. It is a *required* method and not a defaulted one deliberately. A default returning `None` would keep the trait source-compatible for an outside implementer, and would do it by silently switching the collision guard off for anyone who did not override it -- which is exactly the failure this branch already hit once, when a rebase grafted `NoColdPath`'s body onto `ColdPath` and left the whole suite green. Nothing outside this workspace implements the trait today (#266: wayfinder never links devlaunch-core), so the cost is a recompile and the benefit is that the guard cannot be turned off by omission.
|
Fixed, pushed, and green at The case defectYou were right, and the fix is the shape you asked for. Your probe is in as Three more tests came out of it:
On your Something the rebase turned up, worth your attentionMain merged #514 mid-review, which moved So there is now a test that runs the real binary against a real Public API: this widens the promised tier
Snapshots came from CI's regenerate-and-diff, not from guesswork. The non-blocking oneRenamed to Process noteTaken, thank you — the gate I ran requires each of the nine by name, completed and in {success, skipped, neutral}, not a count. Separately: three of the four CI rounds on this branch never started at all, because a The CHANGELOG resolution is verified by #528's own |
#524 widened api's re-exports (29 promised paths to 40), so the promise file is 813 rows where it was 521 and the moved-row figure is 631, not 395. The residual is still 39 types, now over six hundred rows. Note main's checked-in public-api.rest.txt is stale by one row (MetadataStorage::look) against main's own tree; regenerating here adds it.
1 participant
Closes #438.
What was broken
A workspace id is
<repo-slug>-<ref-slug>-<suffix>, and only the four-charactersuffix makes it injective. The readable half is lossy three ways: the owner is not
in it at all,
slug()collapsesfeature/authandfeature-authto one string,and the 47-character cap eats a long ref's tail. Two triples whose ids land on the
same string share both things the id names, because devpod workspace names are
global rather than scoped by repository: one clone directory and one container.
Nothing detected that. The second launch attached to the first one's container and
said nothing, so you opened somebody else's checkout from a command with no reason
to look wrong, and a later
dl <ws> rmon either deleted a clone the other stillclaimed. The suffix width was the whole of the defence, which the two doc comments
in
workspace_id.rssaid in as many words. Both are updated here.The refusal
Both triples and the id, because renaming one of the two branches is the only way
past it and the two rows are the same 47 characters in
dl --ls. The migration'svoice, and the same hazard it already refuses to walk into at migration time.
place_triplescans the records before devpod is asked anything and before thecold path can build a directory, so the guard fires ahead of the attach that does
the damage.
Reached::Nothing, since a refused launch created nothing.The three fail-open rules, each with a test
derivation coerced unsafe refs instead of rejecting them, so a stored branch is
not necessarily one
WorkspaceId::newaccepts; the migration reports such arecord as
unusableand carries on. Swapping theis_ok_andfor anexpectturns the test red.
recorded_id's stated rule.The warm path is untouched
This is the part worth reviewing. Reading the records on the warm attach path is
exactly what devlaunch#145 took off it, and
a_warm_triple_launch_does_no_metadata_io_at_allcaught the first attempt:
MetadataStorage::openquarantines an unusable documentto
.corrupt, so the guard wrote to the cache on a warm launch.So
ColdMachinerygrowsrecorded, which reads through a newMetadataStorage::look: no lock, no migration, noconfig.toml, no clone manager,no write of any kind.
lookis a second constructor rather than a flag onopenbecause all three of
open's writes (creating the cache directory, quarantining anunusable file, backing up one that will not round-trip) exist to protect bytes from
the caller's next save, and a reader has no next save.
NeverColdstill panics if anything opens the machinery, so the 30-odd tests thatassert #145 assert exactly what they asserted before, and
a_launch_that_matches_its_own_record_attaches_and_reads_no_machineryaddsopens == 0on top.Also: the migration half, from the ticket's own comment
migrate_record's guard against adopting another record's clone read aclaimedset built from the pre-run layout alone. Two schema-2 records deriving one
schema-3 id share a destination that no record pointed at before the run, so the
guard never fired: the first renamed onto it, the second found it existing and
repointed itself, and two records ended up owning one clone.
claimednow grows asdestinations are taken, so the second lands in
blockedlike any other conflict.The colliding pair is real
release/999999999999999999999911630andrelease/999999999999999999999911783under
blooop/devlaunchgenuinely derivedevlaunch-release-999999999999999999999911-dq8q,found by searching the shape the module doc records seeing in the wild: long
release refs differing only past the truncation point, so the readable halves match
and only the suffix separates them.
both_of_the_colliding_refs_really_do_derive_one_idis named beside the constants and fails first if a change to the derivation moves
them apart, rather than letting the guard's tests pass against two ids that no
longer collide.
Tests
Core,
flows::launch:both_of_the_colliding_refs_really_do_derive_one_id,a_launch_whose_derived_id_another_triple_already_holds_is_refused,a_launch_that_matches_its_own_record_attaches_and_reads_no_machinery,a_record_whose_branch_is_not_a_legal_ref_does_not_block_a_launch,a_store_that_cannot_be_read_does_not_refuse_a_launch.domain::metadata:a_look_reads_a_corrupt_file_as_empty_and_leaves_it_exactly_as_it_found_it,a_look_at_a_cache_that_is_not_there_creates_nothing,a_look_reads_the_same_records_an_open_does.flows::migration:two_records_that_derive_one_destination_do_not_both_end_up_owning_it.dl::render:an_id_collision_names_both_branches_the_id_and_the_way_out.Public API
public-api.rest.txtonly:LaunchRefusal::IdCollisionand its fields,ColdMachinery::recorded,MetadataStorage::look. Nothing atdevlaunch_core::api, sopublic-api.api.txtis unchanged. Hand-edited, andtherefore provisional until CI's
public-apijob agrees: this host has neither anightly toolchain nor the pinned
cargo-public-api, which docs/development.md saysis the only route there is from here. The partition holds locally; ordering is CI's
to confirm.
Local:
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkandpytest testall green.🤖 Generated with Claude Code
Summary by Sourcery
Refuse launches that would reuse a workspace ID owned by a different triple and prevent the same collision during metadata migration.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: