fix(agent-registry-migration): implement transform.duplicateNames = "suffix" - #1984
Open
dgallitelli wants to merge 2 commits into
Open
Conversation
…suffix" `runtime.transform.duplicateNames` was documented with two modes but only ever had one. The setting was read by nothing: it was not in the Python transform defaults, not in `TransformConfig`, and no code branched on it, so two source records transforming to the same target `(name, recordVersion)` failed identically whether it was set to `fail` or `suffix` -- the second claimant was always refused in `TargetNameClaims.claim`, and a registry with duplicate preview names could not be fully migrated at all. Implementing the mode means answering two separate questions. What name does a record that cannot keep its own get? And which of the records wanting one name is the one that has to move? The first is a pure function of the record: `suffix` moves a record onto `<name>-<sha256(source claimant id)[:8]>`, where the suffix material is the canonical `account/region/registry/recordId` identity of the source record and nothing else -- not the run, the attempt, the batch position or a counter. Names stay within the target's 255-character bound by truncating the base, as the transform's sanitisation fallback does. The second cannot be answered from a per-record view, because it depends on the other records in the registry and on what earlier runs already created. Answering it from arrival order would corrupt data: a re-extract that paginated differently, or an incremental run carrying a different subset, would rename records that are already in the target registry and referenced by name. So it is answered before the load loop starts: * The id map gains a `names` block recording the `name` and `recordVersion` each source record was migrated under, beside the target recordId it already records. Additive and backward compatible: a map written before this reads as "not recorded". * `plan_target_names` (new, used only in `suffix` mode) seeds ownership from that committed state -- including records the current run does not stage at all, whose names an incremental run must not hand to something else -- then re-reads the staged records and gives each remaining contested identity to the lowest canonical claimant id. It runs the same `RecordTransformer.transform` the load will run, so a planned name cannot drift from the claimed one. A record renamed at source releases the identity it no longer holds; a record already holding the suffixed form of the name it still comes with keeps it. It also reserves the suffixed names it hands out, not only the names records asked for, so a third record whose own name happens to be another record's suffixed form is not handed it as well. * `TargetNameClaimPool` carries that plan into the claim as `preferred_name`, so the name a record gets is a function of its own identity and of committed state, never of when it happened to arrive. It also seeds each claim set with the plan as claims already held (in `suffix` only), so the guard *enforces* the plan instead of giving a contested identity to whichever record reaches it first -- the one case where two records are planned onto one identity is the coincidental name above, and without this it would be resolved by staged order. `TargetNameClaims` stays deliberately narrow: it enforces uniqueness over the claims it is given and no longer decides who moves. * The load stage applies the resolved name to the payload, re-checks it against the target request bounds, and records a per-record warning naming it. Only the dedup key moves: `displayName` and the crosswalk's `previewName` keep the name the source record has, so the record stays recognisable. * The target client's own pre-write backstop deliberately stays in `fail` mode: every record a live load writes has already passed through the planned claim, so a collision reaching that point means two source records really do want one identity, and renaming there -- after the lookup that chose create-or-update -- would be a silent overwrite. * `duplicateNames: "fail"` is added to `DEFAULT_TRANSFORM` and to `TransformConfig`, with matching validation on both sides, so a local run and a deployed run agree on the default and an unrecognised value is refused instead of being read as `fail`. What this guarantees, and what it does not. Once a record is in the target registry under a name, every later run leaves it there: the name is committed state, not a re-derived decision. Among records not yet migrated, the lowest source identity keeps the shared name, which is a total order and so gives the same answer for any staged order and any subset of the registry. `suffix` still reduces rather than removes collisions: when both the name a record would take and its own suffixed form are held by other records, that record fails and is reported, with an error naming both identities. The default path is untouched, including its error text: a collision under `fail` still fails that one record, ends the run `PARTIAL_SUCCESS`, and details the record in the failure report. Nothing is planned, and no extra pass over the staged records is made, unless `suffix` is set. Tests, all of which fail on the code before this change: * `WhichRecordKeepsASharedName` in test_load_guards.py -- lowest identity wins, the plan is identical for either staged order, a record already in the registry keeps its name over a lower identity, a name held by a record this run does not stage is not handed out, a suffixed record staged alone does not take the base name back, a map with no recorded names credits a known record with its own name, a record renamed at source releases what it held, the suffixed name a record is moved onto is reserved for it, and a record whose own name is another record's suffixed form keeps it. * `ClaimingThePlannedName` in test_load_guards.py -- the planned owner keeps the name even when it claims second, an established suffixed name is honoured, a stale one is not, the plan is ignored under `fail`, the error when both names are taken names both, a name coinciding with another record's suffixed form resolves identically in all six staged orders of the three records, and there is no suffixed form of an empty name. * `DuplicateNameHandling` in test_load_guards.py -- deterministic suffix across attempts, re-claim stability, 255-character bound, distinct `recordVersion` still not a collision. * test_jobs_end_to_end.py -- both colliding records created under distinct names with `previewName`/`displayName` preserved and a re-run that creates nothing; a suffixed record keeps its name when a later incremental run stages only it; which record keeps a shared name does not change when a re-extract stages them in the other order; a dry run predicts the same names whatever order it reads. * `WhatNameEachRecordWasMigratedUnder` in test_watermark.py -- the `names` block round-trips with its `recordVersion`, merges additively, tolerates individually unusable entries and a map written without it, and rejects a non-object `names`. * `DuplicateNameModes` in test_settings.py -- the new validation. Docs: documented how `suffix` chooses which record is renamed, including the three consequences an operator can be surprised by (the renamed record is not necessarily "the second one", `suffix` can still fail, and a record named like another record's suffixed form keeps its own name); corrected the `duplicateNames` reference entry and three troubleshooting rows that described an extract-stage duplicate-name guard and an extract-summary field that do not exist. Fixes awslabs#1983 Signed-off-by: Davide Gallitelli <davidegallitelli@gmail.com>
Author
|
I don't have permission to apply the |
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.
Issue number: #1983
Concise description of the PR
runtime.transform.duplicateNamesis documented (docs/configuration.md,docs/detaileddoc.md) with two modes,"fail"(default) and"suffix", for the case where two source records share a targetname+recordVersion— legal under the oldbedrock-agentcoreschema, illegal as a dedup key under the newagent-registryone."suffix"was a complete no-op: it was never read by the code that claims target names, so a"suffix"-configured run failed colliding records identically to"fail".This PR implements
"suffix": a colliding record after the first claimant gets a deterministic, suffixed targetname(derived from its own source identity —sha256("{accountId}/{region}/{registryId}/{recordId}")[:8]), while itsdisplayNameand the id-crosswalk'spreviewNamekeep the original name."fail"'s behavior and error text are unchanged (byte-identical, diffed againstmain).Getting this right took two extra rounds past the first pass, because the interesting failure mode isn't "does it work once" — it's "does it stay correct across re-runs and re-orderings":
--incrementalrun that stages only the previously-suffixed record renames it back onto the base name (silently colliding with the record that already holds it), and a full re-extract that receives records in a different pagination order can rename both records. Both reproduced concretely, with no error surfaced (SUCCEEDED, not even a warning).namesblock, backward-compatible with maps that don't have it) and deciding ownership from a total order over source identities before the load loop runs, seeded from every mapping's committed state — not from what happens to be staged this run. This closes both repros, verified by direct re-attempts and by an exhaustive (uncommitted, local-only) sweep over 3375 run/order/subset combinations for a 3-record collision set.ClaimingThePlannedName.test_a_name_that_coincides_with_another_records_suffix_resolves_the_same_in_every_order, all 6 orderings of 3 records) — this one is committed, since it's small and fast.User experience
Before: setting
duplicateNames: "suffix"had no observable effect. A colliding record still failed at load with "Rename one of them in the source registry... and re-extract," regardless of the setting.After: the colliding record loads under a suffixed name (e.g.
python-sort-dict-by-value-a1b2c3d4), keeps that identity across incremental runs and re-extracts regardless of ordering, and itsdisplayNamestays the original, human-readable name. If the suffixed name is also already taken by a different source record,suffixstill fails that record — with an error that now names both identities and both holders, rather than reusingfailmode's text.Known follow-ups (not fixed in this PR, flagged for visibility rather than left silent)
duplicateNamestoDEFAULT_TRANSFORMchanges the config's replay-fingerprint hash, so an extract staged by a pre-this-PR build can't be--resume-loaded by a post-this-PR build. Correct fail-safe (a re-extract is already the documented remediation for any fingerprint mismatch), just worth knowing at upgrade time.suffixback tofailafter a record has already been migrated under a suffixed name, that record fails withfailmode's stock message ("...loading the second would overwrite the first"), which doesn't quite describe the situation (it already exists, under a distinct name). Not incorrect, just imprecise; the fingerprint change above already forces a re-extract on a mode flip, which limits exposure.write_idmap(names=None)silently omits the persisted block. The one caller in this codebase always passes it correctly, but a future caller that forgets to would silently drop the state that suffix-stability depends on. Worth a follow-up to make it required or merge insidewrite_idmapinstead of at call sites.Checklist
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.