treat sync1.1 errors as warnings #989

brianolson · 2025-03-17T18:03:14Z

No description provided.

bnewbold

overall looks good, and a couple good little cleanups along the way.

I flagged a couple places we probably don't need warn-versus-error: #sync events aren't emitted by pre-sync1.1 software at all, and DID syntax in #commit events can always be enforced strictly (not a sync1.1 change)

cmd/relay/bgs/validator.go

bnewbold · 2025-03-18T05:32:30Z

cmd/relay/bgs/validator.go


 	did, err := syntax.ParseDID(msg.Did)
 	if err != nil {
 		syncVerifyErrors.WithLabelValues(hostname, "did").Inc()
-		return nil, err
+		if !val.Sync11ErrorsAreWarnings {


I think all these #sync event checks could be errors-not-warnings. The whole event is new in sync v1.1, so there shouldn't be any problem with old implementations outputting events which don't validate. It is only the #commit events we need to handle with warning-not-error.

this is where I thought the discussion went yesterday, that we have to allow mediocre implementation of sync1.1 for a while

I think the main point is to allow PDS instances which don't implement Sync v1.1 at all, not ones which attempt to implement it but mess up.

aka, we want code which was running and working in December 2024 to keep running for a while longer. but code which was implemented last week and implemented sync1.1 wrong can error.

devinivy · 2025-03-20T16:09:40Z

cmd/relay/bgs/validator.go

 	commit, err := atrepo.LoadCARCommit(ctx, bytes.NewReader([]byte(msg.Blocks)))
 	if err != nil {
 		commitVerifyErrors.WithLabelValues(hostname, "car").Inc()
-		return nil, err
+		if !val.Sync11ErrorsAreWarnings {
+			return nil, err
+		} else {
+			logger.Warn("invalid car", "err", err)
+		}
 	}

 	if commit.Rev != rev.String() {


If there was an error in loading the CAR, I would expect commit to be nil. If that's the case, falling through on the "invalid car" warn would cause a nil pointer exception here checking the rev.

fixed (and in commit handler)

cmd/relay/bgs/validator.go

devinivy · 2025-03-20T19:37:49Z

cmd/relay/bgs/validator.go

 	commit, err := atrepo.LoadCARCommit(ctx, bytes.NewReader([]byte(msg.Blocks)))
 	if err != nil {
 		commitVerifyErrors.WithLabelValues(hostname, "car").Inc()
-		if !val.Sync11ErrorsAreWarnings {
+		if !val.Sync11ErrorsAreWarnings || commit == nil {


I would expect that if err != nil that the commit is going to be nil. Is that not the case? I think that means we always have to error if we fail to read the commit from the CAR, there's no case where we'll end-up warning here.

ok, yeah, content and err are mutually exclusive, cleaned up

bnewbold

there are a few more instances where a check might error, resulting in nil / zero-like values.

especially for the MST inversion case, I think the easiest fix is to split code out to a sub-method which fails or succeeds all together, and then warn-or-error on that overall result.

bnewbold · 2025-03-26T01:47:55Z

cmd/relay/bgs/validator.go

 	}
 	if commit.DID != did.String() {
 		commitVerifyErrors.WithLabelValues(hostname, "did2").Inc()
-		return nil, fmt.Errorf("rev did not match commit")
+		if !val.Sync11ErrorsAreWarnings {
+			return nil, fmt.Errorf("rev did not match commit")


error message should be about DID, not about rev: "message DID and commit DID differed" or something like that

bnewbold · 2025-03-26T01:50:23Z

cmd/relay/bgs/validator.go

+				if !val.Sync11ErrorsAreWarnings {
+					return nil, fmt.Errorf("invalid repo path in ops list: %w", err)
+				} else {
+					logger.Warn("invalid repo path", "err", err)


similar to some of the other branches: if there is a ParseRepoPath error, then nsid and rkey will be empty strings, and the code path below won't work.

I think this could just always be an error: there should be zero or very-near-zero commits floating around the network which would fail this check; not a "Sync v1.1" change.

bnewbold · 2025-03-26T01:51:39Z

cmd/relay/bgs/validator.go

+				if !val.Sync11ErrorsAreWarnings {
+					return nil, err
+				} else {
+					logger.Warn("invalid record cid", "err", err)


I think the warning message should be something like "record CID not found in CAR diff" (not "invalid record cid")

bnewbold · 2025-03-26T01:54:10Z

cmd/relay/bgs/validator.go

+			if !val.Sync11ErrorsAreWarnings {
+				return nil, err
+			} else {
+				logger.Warn("invalid commit ops", "err", err)


as with some of the other code branches, if ParseCommitOps fails, I assume that ops will be nil/empty, and the checks below (like NormalizeOps, and inversion in general) won't make sense.

maybe one way to make this handling simpler would be a "VerifyCommitMessageOps" sub-method. the "Sync11ErrorsAreWarnings" check can be implemented based on the output of that overall function, instead of for each step which might fail.

bnewbold · 2025-03-26T01:55:54Z

cmd/relay/bgs/validator.go


 	did, err := syntax.ParseDID(msg.Did)
 	if err != nil {
 		syncVerifyErrors.WithLabelValues(hostname, "did").Inc()
-		return nil, err
+		if !val.Sync11ErrorsAreWarnings {


I think the main point is to allow PDS instances which don't implement Sync v1.1 at all, not ones which attempt to implement it but mess up.

aka, we want code which was running and working in December 2024 to keep running for a while longer. but code which was implemented last week and implemented sync1.1 wrong can error.

bnewbold · 2025-03-26T01:59:10Z

cmd/relay/bgs/validator.go

+			return nil, err
+		} else {
+			logger.Warn("invalid rev", "err", err)
+		}
 	}
 	if rev.Time().After(time.Now().Add(val.maxRevFuture)) {


rev might be zero-like (empty string) if syntax check above failed

in HandleCommit I'd changed it to assume valid DID and rev TID are basic; propagated that here too

bnewbold · 2025-03-26T01:59:47Z

cmd/relay/bgs/validator.go

+		if val.Sync11ErrorsAreWarnings {
+			logger.Warn("invalid car", "err", err)
+		}
+		return nil, fmt.Errorf("invalid car, %w", err)
 	}

 	if commit.Rev != rev.String() {


commit will be zero-like here if LoadCARCommit failed above

it's okay because we do always error out on that error

bnewbold · 2025-03-26T01:59:54Z

cmd/relay/bgs/validator.go

+			return nil, fmt.Errorf("rev did not match commit")
+		} else {
+			logger.Warn("message rev != commit.rev")
+		}
 	}
 	if commit.DID != did.String() {


commit will be zero-like here if LoadCARCommit failed above

mostly reorg a couple chains of actions into functions so bail out is easier

brianolson · 2025-03-26T15:26:23Z

ok, I think I cleaned up all the places where we should bail on a sequence of steps if there's an error, bundled things up into two new inner functions to organize that.

bnewbold approved these changes Mar 18, 2025

View reviewed changes

treat sync1.1 errors as warnings

e5f3163

brianolson force-pushed the bolson/sync11-error-as-warn branch from d604306 to e5f3163 Compare March 20, 2025 13:59

always require signature

2621ae8

devinivy reviewed Mar 20, 2025

View reviewed changes

cmd/relay/bgs/validator.go Outdated Show resolved Hide resolved

brianolson added 2 commits March 20, 2025 13:39

signatures are errors

991d2f9

if CAR is so bad we can't go on, fail out

6693a5d

devinivy reviewed Mar 20, 2025

View reviewed changes

cleanup car handling

d4ada47

bnewbold reviewed Mar 26, 2025

View reviewed changes

PR feedback

ddccae2

mostly reorg a couple chains of actions into functions so bail out is easier

bnewbold marked this pull request as draft April 23, 2025 20:27

bnewbold closed this Jul 2, 2025

treat sync1.1 errors as warnings #989

treat sync1.1 errors as warnings #989

Uh oh!

Conversation

brianolson commented Mar 17, 2025

Uh oh!

bnewbold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devinivy Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnewbold left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianolson commented Mar 26, 2025

Uh oh!

Uh oh!

devinivy Mar 20, 2025 •

edited

Loading