Helper in daphne_worker_test may lose reports in AggregateStore

[cloudflare]

Helper in daphne_worker_test may lose reports in AggregateStore

[cjpatton]

Thanks for the report, @divergentdave. I believe this is related to #73. My understanding is that the transaction guarantee no longer holds as soon as you have DO->DO communication. This happens here because AggregateStore calls GarbageCollector.

This is not a bug in Miniflare -- Miniflare is correctly emulating production here.

If this is indeed the root cause, then this would get resolved once we use an alarm for garbage collection: ttps://github.com//issues/94

I plan to work on this in the coming weeks (DAP-02 changes are higher priority right now). In the meantime: How frequently does this bug come up? Can you tolerate flakiness for a few weeks?

[branlwyd]

I think we can likely tolerate flakiness for a few weeks. (at worst, we might disable running this integration test by default, until a fix is in place -- not ideal but it'd certainly fix flakiness, and the test is not nearly so flaky that we can't run it manually to ensure we haven't broken Daphne-compatibility while making changes to Janus, which I suppose is my primary concern.)

This happens here because AggregateStore calls GarbageCollector.

Alternatively to disabling the test, would you accept/have time to review a workaround similar to what was put in place for #73? I'd view this as an acceptable tradeoff in test-realism for the time being. (I'm also not sure if this workaround would be as cut-and-dry as the one for #73 -- if not, that would be a good reason not to spend the time implementing it.)

[divergentdave]

I think we've seen it a few times a day in CI, but it's infrequent enough that retries can take care of it.

Hmm, I wonder if the DO->DO communication issue falls under the "with no other intervening I/O" clause, or if there's something else going on. With this particular conflict, the garbage collector is only touched outside of the critical section, at the start of the fetch method.

I notice that the request's body is read between the "agg_share" get and put calls, I wonder if that provides a hole in "the system will prevent concurrent events from executing while awaiting a read operation", such that another DO call can interleave while the first is reading its request body.

[cjpatton]

Alternatively to disabling the test, would you accept/have time to review a workaround similar to what was put in place for #73? I'd view this as an acceptable tradeoff in test-realism for the time being. (I'm also not sure if this workaround would be as cut-and-dry as the one for #73 -- if not, that would be a good reason not to spend the time implementing it.)

Yes, I would review a workaround. I think what we could do is use the environment variable you added last week to disable garbage collection altogether. For example, we would make this line conditional: https://github.com/cloudflare/daphne/blob/main/daphne_worker/src/durable/aggregate_store.rs#L65