fix(server): include nodes and creds in request dedupe#350
fix(server): include nodes and creds in request dedupe#350fallintoplace wants to merge 1 commit into
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Greptile SummaryThis PR fixes request-deduplication collisions in the
Confidence Score: 4/5The change is safe to merge. The dedup logic is now correct and the race fix in Get is sound. The core fix is correct: credentials and node mappings are now included in the request hash, and the credential digest is deterministic (Go's json.Marshal sorts map keys). The Completion copy in Get properly eliminates the concurrent-read race. Two minor concerns: the sync.Once pattern permanently caches a key-generation error with no recovery path, and the deterministic-marshal contract for credential values is implicit. Neither affects correctness under normal operating conditions but could surface in edge cases. pkg/topology/request.go — the lazy sync.Once initialization of the HMAC key and the implicit json.Marshal determinism assumption are worth a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Client
participant S as HTTP Server
participant Q as TrailingDelayQueue
participant W as Timer Worker
Note over S: readRequest resolves effective creds<br/>(payload creds or config fallback)
C->>S: POST /v1/generate (with or without creds/nodes)
S->>S: checkCredentials(payload, config)
S->>Q: Submit(request) → Hash(name+params+HMAC(creds)+canonicalNodes)
Q-->>S: uid (hash)
S-->>C: 202 Accepted + uid
Note over Q,W: Dedup: requests with same logical identity share a queue entry
W->>W: processTopologyRequest
W->>Q: "update Completion{Status:200, Ret:data}"
C->>S: POST /v1/lookup (same effective request)
S->>S: checkCredentials(payload, config)
S->>S: request.Hash() → same uid
S->>Q: Get(uid) → snapshot copy of Completion
Q-->>S: Completion copy (no race on Status/Ret)
S-->>C: 200 OK + topology data
Reviews (1): Last reviewed commit: "fix(server): include nodes and creds in ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func getCredentialHashKey() ([]byte, error) { | ||
| credentialHashKeyOnce.Do(func() { | ||
| credentialHashKey, credentialHashKeyErr = newCredentialHashKey() | ||
| }) | ||
| if credentialHashKeyErr != nil { | ||
| return nil, credentialHashKeyErr | ||
| } | ||
| return credentialHashKey, nil | ||
| } |
There was a problem hiding this comment.
sync.Once
If newCredentialHashKey() returns an error (e.g., crypto/rand.Read fails — possible on constrained or misconfigured systems), credentialHashKeyErr is cached permanently by sync.Once. Every subsequent call to getCredentialHashKey() returns the same error, causing every Hash() call on a request with non-empty credentials to fail. This makes the server permanently unable to process credentialed topology requests until it is restarted, with no recovery path short of a restart.
A more defensive pattern is to initialize the key eagerly during server startup (e.g., in an init() or constructor) and fail fast, rather than caching a fatal error silently in a lazy initializer.
| func getCredentialDigest(creds map[string]any) (string, error) { | ||
| if len(creds) == 0 { | ||
| return "", nil | ||
| } | ||
|
|
||
| data, err := json.Marshal(creds) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to marshal credentials for hashing: %v", err) | ||
| } |
There was a problem hiding this comment.
json.Marshal key ordering is implicit contract for digest determinism
getCredentialDigest relies on json.Marshal sorting map[string]any keys alphabetically to produce a deterministic byte string for the HMAC input. While this behaviour is stable and documented in encoding/json, it is an implicit contract with no code comment. If a credential value is ever a type that does not marshal deterministically (e.g., a map[interface{}]interface{}, a struct with unexported fields, or a type with a non-deterministic MarshalJSON implementation), two logically identical credentials could produce different digests and thus different request hashes, silently breaking deduplication. A brief comment noting the reliance on sorted key marshaling would help future maintainers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
/ok-to-test 4361989 |
|
Hi @fallintoplace , |
2 participants
Description
Fixes request aggregation collisions where requests with the same provider and engine params but different explicit node mappings or credentials could share the same request ID.
The request hash now includes canonicalized node mappings and an HMAC digest of the effective credential map. The server resolves configured credentials before enqueue and lookup, so config-backed requests use the same key for both endpoints.
TrailingDelayQueue.Getalso returns a snapshot of cached completion state so polling cannot race with completion updates.Validation:
make qualify LINTER_BIN=/Users/hoangvu/go/bin/golangci-lintChecklist
git commit -s).