`commandstore` cache poisoning after transient `Get` failure following successful status update
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 1.4k
- Forks
- 364
- Avg merge
- 1d 4h
- Merged PRs (30d)
- 84
Description
Description
commandstore can cache an invalid zero-value Command when backend.Get fails immediately after UpdateStatus succeeds.
Details
pkg/app/server/commandstore/store.go:106-120pkg/app/server/commandstore/cache.go:29-48
If backend.Get fails after UpdateStatus has already succeeded, the error is effectively swallowed and the resulting nil command is passed to the cache.
The nil value is marshaled to JSON as null. When null is later unmarshaled into a non-pointer Command struct, Go treats it as a no-op, leaving a zero-value Command, including PipedId="".
Subsequent Get calls can therefore return a valid-looking zero-value command instead of the actual command.
This can cause ReportCommandHandled to return PermissionDenied for the legitimate piped. Since ReportCommandHandled is also the RPC that could otherwise repair the cache, this can leave manual-approval/cancel commands stuck indefinitely.
Suggested fix
Either:
- return the
backend.Geterror instead of continuing and caching the invalid result, or - skip
cache.Putwhencmd == nil.
Additionally, add a nil guard in commandCache.Put so a nil command cannot be cached accidentally.
Tests
There currently does not appear to be a test file for this package.
Add a regression test covering the following sequence:
UpdateStatussucceeds.- The subsequent
backend.Getfails. - The failed result is not cached.
- A later
Getdoes not return a zero-valueCommand. ReportCommandHandledcontinues to work for the legitimate piped.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading pkg/app/server/commandstore/store.go:106-120 and pkg/app/server/commandstore/cache.go:29-48 to trace the UpdateStatus, backend.Get, and cache.Put sequence. Add a regression test for the failure sequence described in the issue, then run the commandstore package tests. Done means the failed result is not cached, later Get calls do not return a zero-value Command, and ReportCommandHandled still works for the legitimate piped.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100