fix(export): make export clientId collision-safe to prevent concurrent lost-update#10458
Open
davidfirst wants to merge 9 commits into
Open
fix(export): make export clientId collision-safe to prevent concurrent lost-update#10458davidfirst wants to merge 9 commits into
davidfirst wants to merge 9 commits into
Conversation
…t lost-update The clientId is the pending-dir name and the cross-client export lock (export-validate's waitIfNeeded queue sorts pending-dir names; only the first proceeds to validate+persist). A bare Date.now() isn't collision- safe: two exports to the same remote within the same millisecond get the same clientId, share one pending-dir, collapse the queue, and both validate against the pre-persist state, silently losing one update. Append a random suffix so same-ms exports can't collide, keeping the timestamp prefix so the sorted queue still preserves arrival order.
PR Summary by QodoFix export clientId collisions by adding random suffix for concurrent runs Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. @ts-ignore hides TS2345
|
…he new unique format clientId is no longer a pure millisecond timestamp; update the stale PushOptions.clientId doc and the ClientIdInUse error text accordingly.
|
Code review by qodo was updated up to the latest commit 135e445 |
Member
Author
|
Thanks @qodo — both findings were the same point (the now-stale "timestamp in ms" / "exact same millisecond" wording, since
The re-review now reports Bugs (0). (Findings were posted as issue comments, so there's no inline thread to resolve.) |
Thanks — that makes sense.
Looks good to me. |
luvkapur
approved these changes
Jun 26, 2026
|
Code review by qodo was updated up to the latest commit 6e7928c |
…declaration build In dogfooding capsule builds, @teambit/component.sources can be resolved twice — built from source for the isolator capsule vs. the published package that node-modules-linker pulls in from node_modules — so its PackageJsonFile becomes two nominally-distinct-but-structurally-identical classes. The declaration task (tsc --declaration) then rejects passing the isolator's instance to PackageJsonTransformer.applyTransformers, which is typed against the linker's copy (TS2345). Suppress the spurious duplicate-identity mismatch at the seam; the classes are the same shape and version. Unblocks bit_pr after the recent deps/isolator changes.
|
Code review by qodo was updated up to the latest commit cf4dc36 |
|
Code review by qodo was updated up to the latest commit 39be783 |
…rt-clientid-collision # Conflicts: # scopes/component/isolator/isolator.main.runtime.ts
|
Code review by qodo was updated up to the latest commit 75005e9 |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1508
to
1509
| // @ts-ignore | ||
| await PackageJsonTransformer.applyTransformers(component, packageJson); |
There was a problem hiding this comment.
1. @ts-ignore hides ts2345 📘 Rule violation ≡ Correctness
A new // @ts-ignore was added in IsolatorMain to silence a TS2345 PackageJsonFile type mismatch when calling PackageJsonTransformer.applyTransformers(), meaning the change is not typecheck-clean under the repo’s tsc --noEmit expectations. This suppression removes compile-time guarantees at a key integration point and can hide real incompatibilities between two nominally distinct PackageJsonFile definitions caused by duplicate module resolution.
Agent Prompt
## Issue description
A `// @ts-ignore` is suppressing TS2345 at the `PackageJsonTransformer.applyTransformers(...)` call site in `IsolatorMain`, which violates the requirement that changes remain typecheck-clean and removes type safety at an important integration boundary.
## Issue Context
The current setup can result in two nominally distinct `PackageJsonFile` class identities (from duplicate module resolution), making the `packageJson` instance incompatible with `PackageJsonTransformer.applyTransformers()` even though the transformer only needs a small surface area (not the full concrete class). Instead of suppressing the compiler, adjust types so the call is accepted without `@ts-ignore`, ideally by switching the transformer input to a minimal structural type/interface (e.g., based on `packageJsonObject` and `mergePackageJsonObject()`), or using an explicit, well-justified safe cast only if unavoidable.
## Fix Focus Areas
- scopes/component/isolator/isolator.main.runtime.ts[1503-1510]
- scopes/workspace/modules/node-modules-linker/package-json-transformer.ts[17-44]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit a145f3b |
|
Code review by qodo was updated up to the latest commit 6ce30a3 |
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
2 participants
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.
Problem
export'sclientIdis both the pending-dir name and the cross-client export lock —export-validate'swaitIfNeededqueue sorts pending-dir names and lets only the first proceed to validate+persist. It was generated asDate.now().toString(), which isn't collision-safe.When two exports hit the same remote within the same millisecond (e.g. concurrent CI runners pushing the same lane), they get the same clientId, share one pending-dir, collapse the queue to a single entry, and both validate against the pre-persist state — silently losing one runner's update (no divergence detected, no rebase).
This surfaced as an intermittently-failing e2e test (
ci-commands.e2e.ts→ "concurrent runners snapping the SAME component"): both runners exported cleanly and one snap was dropped from the lane. The outcome depended purely on OS scheduling of the two processes.Fix
Append a random suffix to the generated clientId so same-millisecond exports can't collide. The timestamp prefix is kept so the sorted queue still roughly preserves arrival order.
--resumeis unaffected (it echoes back a user-supplied id; clientId is never parsed numerically).