tests: add /debug endpoint to fakestore to allow interrupting downloads - #16881
Merged
bboozzoo merged 7 commits intoApr 13, 2026
Merged
Conversation
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
|
Fri Apr 10 21:16:12 UTC 2026 Failures:Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
bboozzoo
approved these changes
Apr 8, 2026
| mux.Handle("/download/", http.StripPrefix("/download/", http.FileServer(http.Dir(topDir)))) | ||
|
|
||
| fileServer := http.StripPrefix("/download/", http.FileServer(http.Dir(topDir))) | ||
| mux.Handle("/download/", logRangeHeader(store.applyKillAfter(fileServer.ServeHTTP))) |
Contributor
There was a problem hiding this comment.
note to self, we could switch to gorilla mux to at least get the default logger for every request
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16881 +/- ##
==========================================
+ Coverage 77.58% 77.59% +0.01%
==========================================
Files 1369 1368 -1
Lines 189490 189768 +278
Branches 2446 2446
==========================================
+ Hits 147008 147244 +236
- Misses 33601 33639 +38
- Partials 8881 8885 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bboozzoo
added
the
Auto rerun spread
miguelpires
reviewed
Apr 9, 2026
|
|
||
| // endpoint -> quota value, note this is stateful, i.e. the quota is counted | ||
| // for all requests to a given endpoint and after exceeding it, all | ||
| // subsequent requests will fail until it is reset though a request |
Contributor
There was a problem hiding this comment.
Suggested change
| // subsequent requests will fail until it is reset though a request | |
| // subsequent requests will fail until it is reset through a request |
Comment on lines
+376
to
+387
| if kaw.killAfter >= 0 { | ||
| kaw.killAfter -= int64(len(p)) | ||
| } | ||
|
|
||
| if kaw.killAfter < 0 { | ||
| // hijack the connection to force a hard drop | ||
| hj, ok := kaw.ResponseWriter.(http.Hijacker) | ||
| if ok { | ||
| conn, _, _ := hj.Hijack() | ||
| conn.Close() // hard close the TCP connection | ||
| } | ||
| return 0, fmt.Errorf("connection killed") |
Contributor
There was a problem hiding this comment.
Maybe it doesn't matter for the test you want to write, but this doesn't strictly kill the connection after writing up to a threshold, it kills it if the write would exceed the limit. Maybe that's exactly what you need?
Contributor
There was a problem hiding this comment.
Fair point. I think we can close the connection after exactly exceeding the limit. I'll push a patch
…imit Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
miguelpires
reviewed
Apr 10, 2026
| } | ||
|
|
||
| func (kaw *killAfterWriter) Write(p []byte) (int, error) { | ||
| if kaw.killAfter < 0 { |
Contributor
There was a problem hiding this comment.
I think we'll never fall into this case? For instance, if the buffer has 4 bytes and the killAfter is at 2, we'll write 2 bytes and set the killAfter to 0 and close the connection. Subsequent calls won't fall into this and will call Write() with an empty buffer
Suggested change
| if kaw.killAfter < 0 { | |
| if kaw.killAfter <= 0 { |
Contributor
There was a problem hiding this comment.
Ah good point. The net effect should be identical, a Write() with 0 bytes of data is essentially a noop but shouldKill would be set to true anyway and we'd hit the later branch. Anyways, I pushed a tweak and checked the coverage to confirm we're hitting the early exit branch now.
bboozzoo
reviewed
Apr 10, 2026
|
|
||
| kaw := &killAfterWriter{ | ||
| ResponseWriter: w, | ||
| killAfter: killAfter, |
Contributor
There was a problem hiding this comment.
hah, unit tests found a funny race with how the counter is used. AFAIU what happens is that within Write() we may update the internal value of kaw.killAfter and close the connection. The value which is stored in the Store and used to gate a subsequent connection is only updated in a defer function further below which uses kaw.KillAfter as input. So it is entirely possible that in unit tests, after the connection is closed for the first tine, the test can proceed and perform another connection which will be handled before the defer code triggered by previous call had a chance to execute. This will cause next call to observe a value in Store that hasn't yet been updated. In unit tests is is manifested by this failed check:
store_test.go:1354:
// Connection forcefully closed mid-transfer, exactly killAfter bytes received
c.Check(int64(len(got)), Equals, int64(0))
... obtained int64 = 512
... expected int64 = 0
Fix a race in consumign and trackign the left quota. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a new
/debugendpoint to the fakestore, it adds one debug actionkill-requestwhich allows injecting connection dropping after specified number of bytes on a per-path granularity.This is needed to allow testing resuming of partial downloads which is implemented in #16841
JIRA: SNAPDENG-36634