Repository navigation
fix(inventory): see a flock archived while feed usage waits on the item lock - #1026
Conversation
…em lock RecordFeedUsageHandler reads the flock before its transaction and again after the item's FOR UPDATE lock. Both reads were tracked, so EF handed the second read the instance the first had loaded, and a flock archived while the request waited on the lock still passed the eligibility check. IFlockLookup returns snapshots, so its two flock reads now go through untracked repository twins. The tracked reads stay for the lifecycle handlers that mutate the flock.
|
Review of record: Codex PR #1026 reviewReviewed FindingsNo P0–P2 findings. No CONFIRMED or PLAUSIBLE product defect found in this change. The fix closes the reported archive-during-item-lock-wait window, including when the same context already tracks the flock. NitsP3 · CONFIRMED · PR description; The PR description incorrectly says no lookup caller mutates a flock in the same scope and that the simulation seeder reads it once per request. Concrete counterexample: one simulation-seed scope calls This is a description error, not a product defect. The lifecycle handlers save before returning, so later untracked reads see their changes. Both selected simulation tests passed. Replace the blanket claim with the narrower conclusion that the peer handlers do not mutate the flock and the seeder saves its lifecycle changes before subsequent reads. Caller auditAll ten peer handlers consume copied
Paths in the table are relative to The flock-detail and production-report endpoints use one filtered lookup. The other lookup-using endpoints call unchanged display-name reads. The simulation seeder can have both tracked entities and fresh snapshots in one scope, as discussed above; its saved lifecycle sequence remains valid. Tracked repository methods used by lifecycle handlers are unchanged. A fresh lookup can deliberately disagree with an older tracked instance after an external commit. A temporary real-Postgres probe confirmed both lookup methods returned Tenancy, scope and SQLA temporary -- GetByIdAsync / GetReadOnlyAsync
WHERE f."AccountId" = @ef_filter__AccountId
AND (@ef_filter__IsUnrestricted3 OR f."Id" = ANY (@ef_filter__AssignedFlockIds))
AND f."Id" = @id
LIMIT 1
-- GetByIdForFlockScopedWriteAsync / GetReadOnlyForFlockScopedWriteAsync
WHERE f."AccountId" = @accountId AND f."Id" = @id
LIMIT 1Both filtered methods returned only the assigned own-account flock. Both scoped-write methods also returned the own-account unassigned flock, but refused foreign and missing ids. This is the intended #388 bypass of a stale request-start assignment snapshot after the live guard succeeds. All four scoped-write callers still invoke The new row at Regression and mutation evidenceThe test's ordering is deterministic. It acquires the inventory-item row lock before starting the request, observes a waiter through
Logs: Scope and verificationThe eight-file diff is confined to #1022: untracked lookup reads, repository declarations/implementation, the necessary allow-list row, updated test stub, regression test and documentation. The #852 correction accurately removes the claim that tracking is unchanged and explains the stale-instance failure. No schema, dependency, authorization or lifecycle mutation changes.
The full test suite was not rerun. Verification was targeted to the changed behavior and its callers. Verdict: APPROVE; no product defect found, with one non-blocking PR-description nit. |
|
Description corrected per the Astra review's P3 (no code change, no new commit). The blanket claim that no |
1 participant
Closes #1022
Feed usage now refuses a flock that is archived while the request waits on the inventory item lock. Before this change it recorded the usage anyway.
Why it happened
RecordFeedUsageHandlerreads the flock twice. The first read, before the transaction, finds the daily-entry link (#446). The second, after the item'sFOR UPDATElock, checks that the flock can still record production; it exists to catch a deplete or archive that commits while the request is parked on the lock. Both reads went through a tracked query. EF identity resolution returns the instance the change tracker already holds, with the state it had when first loaded, so the second read saw the flock as it was before the transaction opened.The fix
IFlockLookupreturnsFlockDetailssnapshots, never entities, so its reads have no reason to track.FlockLookupnow calls two new untracked repository reads,GetReadOnlyAsyncandGetReadOnlyForFlockScopedWriteAsync. They run the same queries asGetByIdAsyncandGetByIdForFlockScopedWriteAsyncwithAsNoTracking()added. The tracked reads stay as they are for the lifecycle handlers that mutate the flock.The names follow the repo's
GetReadOnlyAsyncconvention, whichTrackedMutationReadTests.AllMutableRepositoryReads_AreTrackedexempts by name. The scoped-write read keepsIgnoreQueryFilters()and reinstatesAccountIdexactly like its tracked twin, so it has its own tenant-bypass allow-list row.Only the feed-usage double read changes behaviour. The peer handlers that call
IFlockLookup(water usage, daily entries, expenses, flock assignment) never mutate the flock. The simulation seeder does mutate flocks in the same scope:EnsureFlockAsynccreates one throughIFlockModule, thenTransitionFlockAsyncreads it through the lookup and runs a tracked lifecycle handler. Each of those writes saves before the next lookup read, so the untracked read returns the values the tracked instance held.Change map
Verification
Test counts, measured at both ends:
main08cf44cThe
mainintegration run had oneSeedCommandTestsdemo-seed case exceed its 60 s budget while still seeding, at load average 11.7. The demo seeder is untouched; that class then passed 11/11 alone.main08cf44c has the same tree as #1021's tested tipfe067c99, where the baseline was taken.dotnet build Cluckwork.slnis clean with warnings as errors. No entity, mapping or migration changes.Regression test
FeedUsageLockOrderTests.FlockArchivedWhileUsageWaitsForTheItemLock_IsRefusedholds the item row lock in a separate connection, waits until the usage request is provably parked behind it (pg_blocking_pids), archives the flock, then releases the lock. It expects422 FeedUsage.FlockNotActiveand no stock consumed.Each mutation below was run against the committed fix, then reverted:
main6e3f584, before refactor(flocks): put flock lifecycle behind an IFlockModule contract #1021, the test fails 3 runs out of 3:200instead of422. It also fails at refactor(flocks): put flock lifecycle behind an IFlockModule contract #1021's tip.FlockLookup.GetForFlockScopedWriteAsyncpointed back at the trackedGetByIdForFlockScopedWriteAsyncgives200, so the test goes RED.AsNoTracking()removed fromGetReadOnlyForFlockScopedWriteAsyncgives200, so the test goes RED.Guards the change had to satisfy:
TenantBypassRealTreeTestsreported the newIgnoreQueryFilterssite. Its allow-list row copies the tracked twin's justification.TrackedMutationReadTests.AllMutableRepositoryReads_AreTrackedexempts reads namedReadOnly, which is why the new reads carry that name. It stays green, and the tracked reads it guards are unchanged.pstack:deslopran on the branch before opening. It found one problem: the allow-list justification listed the four calling handlers, a hand-kept list that would rot. The fix trimmed it to the twin row's wording. A/noslopself-review then rewrote one test comment that narrated history into the constraint it protects.Nothing user-visible changes apart from the refusal itself, so this PR has no screenshots.