fix: improve context propagation

[mschfh]

No description provided.

[Copilot]

Pull Request Overview

This PR improves context propagation throughout the codebase by adding context.Context as the first parameter to all datastore interface methods and their implementations. The change enables better request tracing, cancellation handling, and timeout management across database operations.

Key changes:

• Modified the Datastore interface and all implementations to accept context.Context
• Updated all function calls to pass context through the call stack
• Replaced hardcoded context.TODO() and context.Background() calls with propagated context

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
datastore/datastore.go	Updated interface to include context parameter in all methods
datastore/sync_entity.go	Modified all datastore methods to accept and use context
datastore/item_count.go	Added context parameter to item count operations
datastore/instrumented_datastore.go	Updated Prometheus wrapper to propagate context
datastore/datastoretest/mock_datastore.go	Updated mock implementations with context parameter
datastore/sync_entity_test.go	Modified tests to pass context.Background()
datastore/item_count_test.go	Modified tests to pass context.Background()
command/command.go	Updated command handlers to propagate context
command/server_defined_unique_entity.go	Added context parameter and propagated through calls
command/command_test.go	Modified tests to pass context.Background()
command/server_defined_unique_entity_test.go	Modified tests to pass context.Background()
controller/controller.go	Updated to pass request context to command handler
middleware/disabled_chain.go	Updated to pass context to IsSyncChainDisabled
middleware/middleware_test.go	Fixed mock expectations to include context parameter and removed duplicate recorder initialization
go.mod	Moved smithy-go to indirect dependencies

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[DJAndries]

Arg... this line makes nervous around adding context propagation to commits as well... If the request is cancelled before this line is reached, then we will never update the cached latest mtime if entities were inserted/updated before the cancellation, resulting in a higher risk of client conflicts... It may be safer to leave out propagation for commits, unfortunately. At least until we make this operation atomic.

[mschfh]

The current implementation retries for several minutes, long after the client abandoned (and possibly retried) the request (while the first request is still processing in the background).

It may be safer to leave out propagation for commits, unfortunately.

Considering the above, having those race conditions seems more risky at first glance?

If the context is cancelled, wouldn't the client consider the write request unsuccessful and retry it?

At least until we make this operation atomic.

Avoiding dual-writes seems difficult with the current architecture, are you referring to the SQL refactor?

[DJAndries]

If the context is cancelled, wouldn't the client consider the write request unsuccessful and retry it?

The ResetProgressMarkerOnCommitFailures browser feature enables a conflict recovery procedure which involves fetching all items for a given data type. If we don't update the cached mtime during the cancelled request, then the client may not be able to fetch the items inserted during that request resulting in recovery failure.

Avoiding dual-writes seems difficult with the current architecture, are you referring to the SQL refactor?

Yes, in the refactor we can use transactions to ensure atomicity by not completing the write if cancelled.

[github-actions]

[puLL-Merge] - brave/go-sync@374

Description

This PR propagates context.Context through the codebase, adding it as the first parameter to datastore and cache methods. This is a standard Go practice that enables proper request cancellation, timeout handling, and passing request-scoped values through the call chain. The change updates the Datastore interface, all its implementations (including mocks and instrumented wrappers), and all callers to pass context appropriately.

Additionally, the auto-generated instrumented wrappers (instrumented_redis.go and instrumented_datastore.go) have been regenerated with updated gowrap configuration.

Possible Issues

1. Inconsistent context usage in HandleClientToServerMessage: The function receives a ctx parameter but uses context.TODO() for handleCommitRequest (line 463) and context.Background() for handleClearServerDataRequest (line 477) instead of passing the received ctx. This defeats the purpose of context propagation for these code paths.

2. Variable shadowing removed for conflict and deleted: In handleCommitRequest, the variables conflict and deleted were previously declared at the loop scope level (line 259-260 in old code). Now they are declared inline within conditional blocks. While this works, it changes the scoping behavior and the variables are no longer accessible after their respective blocks.

Security Hotspots

None identified.

Privacy Hotspots

None identified.

Changes

Changes

cache/instrumented_redis.go

• Regenerated file with updated gowrap template
• Reordered Incr and Set methods (alphabetical ordering by gowrap)
• Updated generation comment format

command/command.go

• Added ctx context.Context as first parameter to handleGetUpdatesRequest, handleCommitRequest, handleClearServerDataRequest, HandleClientToServerMessage, getItemCounts, and getInterimItemCounts
• Updated all datastore and cache method calls to include context
• Introduced interimErr variable to separate interim count errors from other errors
• Issue: Uses context.TODO() and context.Background() instead of passed ctx in some places

command/server_defined_unique_entity.go

• Added ctx context.Context parameter to InsertServerDefinedUniqueEntities
• Updated datastore calls to include context

controller/controller.go

• Passes request context to HandleClientToServerMessage

datastore/datastore.go

• Added context.Context as first parameter to all interface methods

datastore/datastoretest/mock_datastore.go

• Updated mock implementations to accept and pass context

datastore/instrumented_datastore.go

• Regenerated with context parameters

datastore/item_count.go, datastore/sync_entity.go

• Updated all method implementations to accept and use context
• Replaced context.TODO() calls with the passed context

middleware/disabled_chain.go

• Passes context to IsSyncChainDisabled

Test files

• Updated all test calls to include context.Background()

sequenceDiagram
    participant Client
    participant Controller
    participant Command
    participant Cache
    participant Datastore

    Client->>Controller: HTTP Request
    Controller->>Controller: Extract context from request
    Controller->>Command: HandleClientToServerMessage(ctx, ...)
    
    alt GET_UPDATES
        Command->>Command: handleGetUpdatesRequest(ctx, ...)
        Command->>Datastore: GetUpdatesForType(ctx, ...)
        Datastore-->>Command: entities
        Command->>Cache: IsTypeMtimeUpdated(ctx, ...)
        Cache-->>Command: bool
        Command->>Cache: SetTypeMtime(ctx, ...)
    else COMMIT
        Command->>Command: handleCommitRequest(ctx, ...)
        Command->>Datastore: GetClientItemCount(ctx, ...)
        Command->>Datastore: InsertSyncEntity(ctx, ...) / UpdateSyncEntity(ctx, ...)
        Command->>Cache: IncrementInterimCount(ctx, ...)
        Command->>Datastore: UpdateClientItemCount(ctx, ...)
    else CLEAR_SERVER_DATA
        Command->>Command: handleClearServerDataRequest(ctx, ...)
        Command->>Datastore: DisableSyncChain(ctx, ...)
        Command->>Datastore: ClearServerData(ctx, ...)
        Command->>Cache: Del(ctx, ...)
    end
    
    Command-->>Controller: response
    Controller-->>Client: HTTP Response

Loading

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.