feat!: cross-save resistant configFile #959

mshanemc · 2023-10-13T15:30:10Z

requires salesforcecli/plugin-data#721

Primary: make configFile save from cross-saving

every property in a file is now a Last-Write-Wins Register on a Conflict Free Replicated Data Type, and BaseConfigStore now holds the CRDT
- they record timestamps, and the more recent timestamp wins
when a ConfigFile is read from fs, the fs modstamp is used as the timestamp for every property in the CRDT
deletes (removed properties) are handled with a special tombstone in their state
lockfiles (same pattern uses to fix the alias cross-save problem) are used in the write/writeSync methods

BREAKING CHANGE: AuthInfo.getFields now returns a read-only object. Use AuthInfo.update to change values in the fields.

BREAKING CHANGE: setContents method is no longer available in the ConfigFile stack.
BREAKING CHANGE:awaitEach is removed from ConfigFile stack
BREAKING CHANGE: write(sync) method no longer accepts a param. Use other methods (set, unset) to make modifications, then call write()/writeSync() to do the write.
BREAKING CHANGE: the use of lodash-style get/set (ex: set('foo.bar.baz[0]', 3) no longer works.
BREAKING CHANGE: You can no longer override the setMethod and getMethod when extending classes built on ConfigFile. Technically you could override get/set, but DON'T!
BREAKING CHANGE: everything related to tokens/tokenConfig is gone.

Other changes

BREAKING CHANGE: node18+ only, compiles to es2022
move uniqid to a shared function, outside of testSetup

Remove previously deprecated stuff since this is a major release

BREAKING CHANGE: removed sfdc.isInternalUrl. Use new SfdcUrl(url).isInternalUrl()
BREAKING CHANGE: removed sfdc.findUppercaseKeys. There is no replacement.
BREAKING CHANGE: removed SchemaPrinter. There is no replacement.

also ttypescript => ts-patch @W-14388558@

TODO:

make sure this publishes a major version
setContents, especially with sfdxConfig.merge
plugin-data adjustments
what to do about getMethod/setMethod
can we do without the CRDT peer/id stuff?
local NUTs for massive parallelization of stuff hitting the same file
migration docs
check command perf in plugin-settings

What issues does this PR fix or reference?

forcedotcom/cli#2423
forcedotcom/cli#2528
@W-14085765@

Release notes (CLI)

Sometimes the CLI would try to save 2 copies of the same file concurrently, resulting in either an invalid file (busted JSON) or one of the "savers" losing their changes. We've redesigned how files are saved to try to be safer about concurrency.

QA highlights

The code for sfdxConfig interop changed quite a bit. Make sure it's still reading/writing that file correctly.
Compare perf in things like plugin-settings and plugin-auth where we're writing to these files. the lock stuff could introduce some extra latency.

Check cases (ex: no file exists)

Things outside of core that use ConfigFile to test with

deployCache and manifestCache in PDR
scratch and sandbox caches in plugin-org
something in plugin-data for supporting --use-most-recent
remoteSourceTrackingService in STL
most of plugin-settings (config and alias)

The Config family of classes' write(sync) method used to accept a param. The value in the param would overwrite the existing file.

…core into sm/crdt-config

MIGRATING_V5-V6.md

shetzel · 2023-10-31T03:23:53Z

src/config/config.ts


-    if (value == null) {
-      delete content[propertyName];
+    if (value == null || value === undefined) {


Since value == null uses the double equals, adding || value === undefined is redundant.

src/config/config.ts

shetzel · 2023-10-31T03:41:41Z

src/config/config.ts

+    await fs.promises.mkdir(pathDirname(path), { recursive: true });
+    await fs.promises.writeFile(path, JSON.stringify(translated, null, 2));
+  } catch (error) {
+    /* Do nothing */


Are you 100% positive that swallowing this error will not cause future headaches? Should the error be logged?

No, but I am 100% positive it currently does that. https://github.com/forcedotcom/sfdx-core/pull/959/files/19e4679631ec94d5bbd74ae7e87ff21ac6447f55#diff-615dacd7000fa06d7afef0f22b7b6d9267c6219fbfc9fa0315e51a5f5d963755L629

Logging them sounds like a good idea.

src/config/config.ts

shetzel · 2023-10-31T03:54:01Z

src/config/configFile.ts

+
+    // unlock the file
+    if (typeof unlockFn !== 'undefined') {
+      await unlockFn();


should unlocking be done in a finally where the lock occurs?

If I put the writeFile in a try/finally I think that takes care of the only other place it could fail. See change.

shetzel · 2023-10-31T04:06:41Z

src/config/lwwMap.ts

+  [Property in keyof P]: LWWRegister<P[Property] | typeof SYMBOL_FOR_DELETED>['state'];
+};
+
+/**


sfdx-core has generated docs from code so I think it should be added here.

mdonnalley · 2023-10-31T15:45:15Z

QA

Using https://github.com/mdonnalley/crdt-config-test (and yarn link @salesforce/core)

single process

bin/dev.js crdt --operation set --key foo --value bar
🟢 Writes new value to config

bin/dev.js crdt --operation update --key foo --value baz
🟢 Updates value in config

bin/dev.js crdt --operation unset --key foo
🟢 Removes value from config

two processes

🟢 process 1 sets new key last

bin/dev.js crdt --operation set --key foo --value process1 --pre-change-wait 1000 & bin/dev.js crdt --operation set --key foo --value process2 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 25373
[2] 25374
[25374] Reading config file after 0ms...
[25373] Reading config file after 0ms...
[25374] config file: /Users/mdonnalley/.sfdx/crdt.json
[25373] config file: /Users/mdonnalley/.sfdx/crdt.json
[25374] Changing config file after 0ms...
[25374] SET: foo=process2
[25374] Writing config file after 0ms...
[25374] Done!
[2]  + 25374 done       bin/dev.js crdt --operation set --key foo --value process2
[25373] Changing config file after 1000ms...
[25373] SET: foo=process1
[25373] Writing config file after 0ms...
[25373] Done!
[1]  + 25373 done       bin/dev.js crdt --operation set --key foo --value process1 --pre-change-wait
"process1"

🟢 process 2 sets new key last

bin/dev.js crdt --operation set --key foo --value process1  & bin/dev.js crdt --operation set --key foo --value process2 --pre-change-wait 1000 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 25434
[2] 25435
[25435] Reading config file after 0ms...
[25435] config file: /Users/mdonnalley/.sfdx/crdt.json
[25434] Reading config file after 0ms...
[25434] config file: /Users/mdonnalley/.sfdx/crdt.json
[25434] Changing config file after 0ms...
[25434] SET: foo=process1
[25434] Writing config file after 0ms...
[25434] Done!
[1]  - 25434 done       bin/dev.js crdt --operation set --key foo --value process1
[25435] Changing config file after 1000ms...
[25435] SET: foo=process2
[25435] Writing config file after 0ms...
[25435] Done!
[2]  + 25435 done       bin/dev.js crdt --operation set --key foo --value process2 --pre-change-wait
"process2"

🟢 process 1 sets new key last, process 2 does the last config.write

bin/dev.js crdt --operation set --key foo --value process1 --pre-change-wait 1000 & bin/dev.js crdt --operation set --key foo --value process2 --pre-write-wait 5000 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 25803
[2] 25804
[25803] Reading config file after 0ms...
[25804] Reading config file after 0ms...
[25803] config file: /Users/mdonnalley/.sfdx/crdt.json
[25804] config file: /Users/mdonnalley/.sfdx/crdt.json
[25804] Changing config file after 0ms...
[25804] SET: foo=process2
[25803] Changing config file after 1000ms...
[25803] SET: foo=process1
[25803] Writing config file after 0ms...
[25803] Done!
[1]  - 25803 done       bin/dev.js crdt --operation set --key foo --value process1 --pre-change-wait
[25804] Writing config file after 5000ms...
[25804] Done!
[2]  + 25804 done       bin/dev.js crdt --operation set --key foo --value process2 --pre-write-wait
"process1"

🟢 process 2 sets new key last, process 1 does the last config.write

bin/dev.js crdt --operation set --key foo --value process1 --pre-write-wait 5000 & bin/dev.js crdt --operation set --key foo --value process2 --pre-change-wait 1000 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 26063
[2] 26064
[26064] Reading config file after 0ms...
[26064] config file: /Users/mdonnalley/.sfdx/crdt.json
[26063] Reading config file after 0ms...
[26063] config file: /Users/mdonnalley/.sfdx/crdt.json
[26063] Changing config file after 0ms...
[26063] SET: foo=process1
[26064] Changing config file after 1000ms...
[26064] SET: foo=process2
[26064] Writing config file after 0ms...
[26064] Done!
[2]  + 26064 done       bin/dev.js crdt --operation set --key foo --value process2 --pre-change-wait
[26063] Writing config file after 5000ms...
[26063] Done!
[1]  + 26063 done       bin/dev.js crdt --operation set --key foo --value process1 --pre-write-wait
"process2"

🔴 process 1 sets new key then process 2 deletes it, process 1 does final config.write

bin/dev.js crdt --operation set --key foo --value process1 --pre-write-wait 5000 & bin/dev.js crdt --operation unset --key foo --pre-change-wait 1000 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 26969
[2] 26970
[26969] Reading config file after 0ms...
[26970] Reading config file after 0ms...
[26969] config file: /Users/mdonnalley/.sfdx/crdt.json
[26970] config file: /Users/mdonnalley/.sfdx/crdt.json
[26969] Changing config file after 0ms...
[26969] SET: foo=process1
[26970] Changing config file after 1000ms...
[26970] UNSET: foo
[26970] Writing config file after 0ms...
[26970] Done!
[2]  + 26970 done       bin/dev.js crdt --operation unset --key foo --pre-change-wait 1000
[26969] Writing config file after 5000ms...
[26969] Done!
[1]  + 26969 done       bin/dev.js crdt --operation set --key foo --value process1 --pre-write-wait
"process1"

It should have been null since process 2 had the later change

🟢 process 1 sets new key then process 2 deletes it, process 2 does final config.write

bin/dev.js crdt --operation set --key foo --value process1 & bin/dev.js crdt --operation unset --key foo --pre-change-wait 1000 --pre-write-wait 5000 & wait && jq .foo /Users/mdonnalley/.sfdx/crdt.json
[1] 27390
[2] 27391
[27391] Reading config file after 0ms...
[27390] Reading config file after 0ms...
[27391] config file: /Users/mdonnalley/.sfdx/crdt.json
[27390] config file: /Users/mdonnalley/.sfdx/crdt.json
[27390] Changing config file after 0ms...
[27390] SET: foo=process1
[27390] Writing config file after 0ms...
[27390] Done!
[1]  - 27390 done       bin/dev.js crdt --operation set --key foo --value process1
[27391] Changing config file after 1000ms...
[27391] UNSET: foo
[27391] Writing config file after 5000ms...
[27391] Done!
[2]  + 27391 done       bin/dev.js crdt --operation unset --key foo --pre-change-wait 1000  5000
null

mdonnalley · 2023-10-31T18:46:29Z

QA (sf specific)

config/alias

yarn link @salesforce/core && sf plugins link --no-install in plugin-settings

in dreamhouse-lwc
🟢 sf config list
🟢 sf config set org-metadata-rest-deploy=true --global
🟢 sf config set org-metadata-rest-deploy=true
🟡 sf config set org-metadata-rest-deploy=true & sf config set org-metadata-rest-deploy=false & wait (non-determinant which one wins but that's just because we can't predict which process does the final change)
🟢 sf config unset org-metadata-rest-deploy
🟢 sf config unset org-metadata-rest-deploy --global
🟢 sf alias list
🟢 sf alias set foo=bar
🟢 sf alias unset foo

org create

yarn link @salesforce/core && sf plugins link --no-install in plugin-org

in dreamhouse-lwc
🟢 sf org create scratch --edition developer --alias qa -f config/project-scratch-def.json --set-default

deploy/retrieve

yarn link @salesforce/core && sf plugins link --no-install in plugin-deploy-retrieve

in dreamhouse-lwc
🟢 sf project deploy start --source-dir force-app
🟢 sf project deploy resume --use-most-recent
🟢 sf project deploy start --source-dir force-app --async
🟢 sf project deploy report --use-most-recent
cleared source tracking using sf project tracking delete
🟢 sf project deploy start --ignore-conflicts
🟢 sf project deploy resume --use-most-recent
created new custom object in org
🟢 sf project retrieve start

feat: reusable file locks outside of ConfigFile

refactor: dev-scripts

feat: ts5 and ts-patch

mshanemc added 19 commits October 9, 2023 13:24

feat(wip): crdt-configfile

bad6dfb

refactor: share lockfile options

20a6c90

feat: configFile file locks and write-via-merge

91316a7

test: mocks for new stat/read changes

947b3df

test(wip): mocks for lockfile, but also some TBD skips

03a7b8e

test: how are the nuts

e7e23bb

feat!: remove parameters on config.write

8e346ca

The Config family of classes' write(sync) method used to accept a param. The value in the param would overwrite the existing file.

refactor: defaults via spread

cdd36fb

refactor: configFile.write handles file creation

dddd83b

test: soiGenerator uses testSetup for projects, auth

a039b7f

feat: typing for what goes in the org-user file

bde5005

test: un-skip more tests

a36f946

refactor: set configs username prop, no mutating

13dd085

test: remove more skips

469373f

test: ttlConfig expiration logic

57d56ce

feat!: make AuthInfo.getFields return readonly

41423d6

refactor: restore testSetup.uniqid export

c5926f0

chore: bump dev-scripts

2481288

chore: bump jsforce deps

4e02b0a

mshanemc changed the title ~~chore: bump jsforce deps~~ feat!: cross-save resistant configFile Oct 13, 2023

mshanemc and others added 10 commits October 13, 2023 10:59

Merge remote-tracking branch 'origin/main' into sm/crdt-config

9dff8f0

chore(release): 6.0.1-crdt.0 [skip ci]

70633b9

chore: use v5 for consistent resolutions

e4407a7

Merge branch 'sm/crdt-config' of https://github.com/forcedotcom/sfdx-…

51c1b9f

…core into sm/crdt-config

chore(release): 5.4.0-crdt.1 [skip ci]

a45bbf5

refactor: further reduce contents replacement

6904cf2

chore: downgrade jsforce

f6f677f

Merge branch 'sm/crdt-config' of https://github.com/forcedotcom/sfdx-…

079d73b

…core into sm/crdt-config

ci: don't update jsforce to latest

5a85d53

ci: pdr tracking xnuts

f3abda1

Merge remote-tracking branch 'origin/main' into sm/crdt-config

19e4679

mshanemc requested a review from shetzel October 27, 2023 22:09

shetzel reviewed Oct 31, 2023

View reviewed changes

mshanemc added 2 commits October 31, 2023 09:28

Merge remote-tracking branch 'origin/main' into sm/crdt-config

e36e102

refactor: steve review

9d99e61

shetzel previously approved these changes Oct 31, 2023

View reviewed changes

mdonnalley previously approved these changes Oct 31, 2023

View reviewed changes

mshanemc added 2 commits November 8, 2023 15:06

feat: reusable file locks outside of ConfigFile

29c59e0

Merge pull request #981 from forcedotcom/sm/reusable-file-locks

5562a3d

feat: reusable file locks outside of ConfigFile

mshanemc dismissed stale reviews from mdonnalley and shetzel via 5562a3d November 9, 2023 17:52

mshanemc and others added 9 commits November 9, 2023 15:58

Merge remote-tracking branch 'origin/main' into sm/crdt-config

a702f2b

refactor: dev-scripts

9fe6f49

test: remove plugin-env from external nuts (it has none)

7a940f3

test: add apex, comunity nuts

3e9c988

ci: also remove core from apex-node lib

d5b345f

Merge pull request #982 from forcedotcom/sm/dev-scripts

a57f79f

refactor: dev-scripts

Merge remote-tracking branch 'origin/main' into sm/crdt-config

4dbd64a

feat: ts5 and ts-patch

4e66905

Merge pull request #983 from forcedotcom/sm/ts-patch

2b6556b

feat: ts5 and ts-patch

mdonnalley previously approved these changes Nov 13, 2023

View reviewed changes

mshanemc added 2 commits November 13, 2023 13:45

Merge remote-tracking branch 'origin/main' into sm/crdt-config

82c4e18

chore: bump major

515b5a8

mshanemc dismissed mdonnalley’s stale review via 515b5a8 November 13, 2023 19:45

mshanemc marked this pull request as ready for review November 13, 2023 19:46

refactor: revert #980 in favor of a jsforce solution

ac78de4

mshanemc merged commit 6836868 into main Nov 14, 2023
39 of 67 checks passed

mshanemc deleted the sm/crdt-config branch November 14, 2023 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: cross-save resistant configFile #959

feat!: cross-save resistant configFile #959

mshanemc commented Oct 13, 2023 •

edited

Loading

shetzel Oct 31, 2023

shetzel Oct 31, 2023

mshanemc Oct 31, 2023

shetzel Oct 31, 2023

mshanemc Oct 31, 2023

shetzel Oct 31, 2023

mdonnalley commented Oct 31, 2023

mdonnalley commented Oct 31, 2023

feat!: cross-save resistant configFile #959

feat!: cross-save resistant configFile #959

Conversation

mshanemc commented Oct 13, 2023 • edited Loading

Primary: make configFile save from cross-saving

Other changes

Remove previously deprecated stuff since this is a major release

What issues does this PR fix or reference?

Release notes (CLI)

QA highlights

shetzel Oct 31, 2023

Choose a reason for hiding this comment

shetzel Oct 31, 2023

Choose a reason for hiding this comment

mshanemc Oct 31, 2023

Choose a reason for hiding this comment

shetzel Oct 31, 2023

Choose a reason for hiding this comment

mshanemc Oct 31, 2023

Choose a reason for hiding this comment

shetzel Oct 31, 2023

Choose a reason for hiding this comment

mdonnalley commented Oct 31, 2023

single process

two processes

mdonnalley commented Oct 31, 2023

config/alias

org create

deploy/retrieve

mshanemc commented Oct 13, 2023 •

edited

Loading